Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IO: use Location, TypedIOService #17

Merged
merged 2 commits into from
Aug 13, 2020
Merged

IO: use Location, TypedIOService #17

merged 2 commits into from
Aug 13, 2020

Conversation

frauzufall
Copy link
Member

This PR depends on the following PRs in scijava-common:

It makes the IO classes to use Location and the TypedIOService. It also sneaks in a small change to the TableIOOptions where the parser is now of type Function<String, ?> instead of Function<String, Object>.

@imagejan imagejan marked this pull request as ready for review August 13, 2020 18:19
@imagejan imagejan merged commit 7d670c7 into master Aug 13, 2020
@imagejan imagejan deleted the use-location branch August 13, 2020 18:21
@imagejan
Copy link
Member

Lovely, thanks @frauzufall!

One question though: should we keep (i.e. re-introduce) the open and save methods without TableIOOptions, creating an empty TableIOOptions object with default options?

Also, I think a version bump to 0.7.0 is required now according to SemVer, right?

@frauzufall
Copy link
Member Author

frauzufall commented Aug 13, 2020

@imagejan thanks for reviewing!! open and save methods without TableIOOptions are still there, coming from the TypedIOService<Table<?, ?>> interface, see test. Or do you mean something else?

I agree with 0.7.0. I made TableIOPlugin an interface - which I think makes sense. I thought it would be not much more than that and changing the parser class in TableIOOptions, but revapi with "exclude": ["org.scijava.table.io.TableIOPlugin", "org.scijava.table.io.TableIOOptions", "org.scijava.table.io.ColumnTableIOOptions"] (this tool is 💯) prints more:

[ERROR] java.method.parameterTypeChanged: parameter boolean org.scijava.io.AbstractTypedIOService<D>::canSave(===D===, java.lang.String) @ org.scijava.table.io.DefaultTableIOService: The type of the parameter changed from 'org.scijava.table.Table<?, ?>' to 'D extends java.lang.Object'.
[ERROR] java.method.abstractMethodAdded: method java.lang.Class<PT> org.scijava.plugin.PTService<PT extends org.scijava.plugin.SciJavaPlugin>::getPluginType() @ org.scijava.table.io.DefaultTableIOService: Abstract method was added.
[ERROR] java.method.abstractMethodAdded: method java.lang.Class<T> org.scijava.Typed<T>::getType() @ org.scijava.table.io.DefaultTableIOService: Abstract method was added.
[ERROR] java.method.parameterTypeChanged: parameter void org.scijava.io.AbstractTypedIOService<D>::save(===D===, java.lang.String) throws java.io.IOException @ org.scijava.table.io.DefaultTableIOService: The type of the parameter changed from 'org.scijava.table.Table<?, ?>' to 'D extends java.lang.Object'.
[ERROR] java.method.addedToInterface: method boolean org.scijava.io.TypedIOService<D>::canOpen(org.scijava.io.location.Location) @ org.scijava.table.io.TableIOService: Method was added to an interface.
[ERROR] java.method.parameterTypeChanged: parameter boolean org.scijava.io.TypedIOService<D>::canSave(===D===, java.lang.String) @ org.scijava.table.io.TableIOService: The type of the parameter changed from 'org.scijava.table.Table<?, ?>' to 'D extends java.lang.Object'.
[ERROR] java.method.addedToInterface: method boolean org.scijava.io.TypedIOService<D>::canSave(D, org.scijava.io.location.Location) @ org.scijava.table.io.TableIOService: Method was added to an interface.
[ERROR] java.method.addedToInterface: method <P extends PT extends org.scijava.plugin.SingletonPlugin> P org.scijava.plugin.SingletonService<PT extends org.scijava.plugin.SingletonPlugin>::getInstance(java.lang.Class<P>) @ org.scijava.table.io.TableIOService: Method was added to an interface.
[ERROR] java.method.addedToInterface: method java.util.List<PT> org.scijava.plugin.HandlerService<DT, PT extends org.scijava.plugin.HandlerPlugin<DT extends java.lang.Object>>::getInstances() @ org.scijava.table.io.TableIOService: Method was added to an interface.
[ERROR] java.method.addedToInterface: method org.scijava.table.Table<?, ?> org.scijava.table.io.TableIOService::open(org.scijava.io.location.Location, org.scijava.table.io.TableIOOptions) throws java.io.IOException: Method was added to an interface.
[ERROR] java.method.parameterTypeChanged: parameter void org.scijava.io.TypedIOService<D>::save(===D===, java.lang.String) throws java.io.IOException @ org.scijava.table.io.TableIOService: The type of the parameter changed from 'org.scijava.table.Table<?, ?>' to 'D extends java.lang.Object'.
[ERROR] java.method.addedToInterface: method void org.scijava.table.io.TableIOService::save(org.scijava.table.Table<?, ?>, org.scijava.io.location.Location, org.scijava.table.io.TableIOOptions) throws java.io.IOException: Method was added to an interface.

Sorry for not checking this earlier - I will have a closer look tomorrow. Adding more default implementations to TableIOService would be more backwards compatible. But also some of these entries look weird to me, like

[ERROR] java.method.parameterTypeChanged: parameter void org.scijava.io.TypedIOService<D>::save(===D===, java.lang.String) throws java.io.IOException @ org.scijava.table.io.TableIOService: The type of the parameter changed from 'org.scijava.table.Table<?, ?>' to 'D extends java.lang.Object'.

@imagejan
Copy link
Member

@frauzufall wrote:

open and save methods without TableIOOptions are still there, coming from the TypedIOService<Table> interface, see test.

Ah, yes, I missed that, thanks!

(this tool is 💯)

Agreed. I actually ran mvn revapi:report (which generates a nice summary webpage) and mvn revapi:update-versions and it suggested the 0.7.0 bump itself : )

Will cut a release shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants