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.

Sign up for GitHub

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

Jump to bottom

Use Location in the I/O API #395

Merged
merged 6 commits into from
Aug 13, 2020
Merged

Use Location in the I/O API #395

merged 6 commits into from
Aug 13, 2020

Conversation

frauzufall
Copy link
Member

Following this gitter discussion I cherry-picked this commit and re-added the String methods to hopefully keep backwards compatibility.

I also added a test, but it's a bit dull. I forgot how to include a test SciJavaPlugin without having to define a public class, can someone remind me?

Happy for feedback!

@ctrueden @frauzufall
Use Location, not String, in the I/O API
b7285c5 
This is much more elegant. It is also a very breaking change.

Co-authored-by: Deborah Schmidt <[email protected]>
@frauzufall
Copy link
Member Author

Here is what revapi has to say when comparing this branch to 2.83.3:

[ERROR] Failed to execute goal org.revapi:revapi-maven-plugin:0.11.4:check (default-cli) on project scijava-common: The following API problems caused the build to fail:
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.io.AbstractIOPlugin<D extends java.lang.Object>: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.io.AbstractIOPlugin<D extends java.lang.Object>: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.io.AbstractIOPlugin<D extends java.lang.Object>: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.io.DefaultIOService: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.io.DefaultIOService: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.method.returnTypeTypeParametersChanged: method java.lang.Class<org.scijava.io.location.Location> org.scijava.io.IOPlugin<D>::getType(): The return type changed from 'java.lang.Class<java.lang.String>' to 'java.lang.Class<org.scijava.io.location.Location>'.
[ERROR] java.class.noLongerImplementsInterface: interface org.scijava.io.IOPlugin<D extends java.lang.Object>: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.method.returnTypeTypeParametersChanged: method java.lang.Class<org.scijava.io.location.Location> org.scijava.io.IOService::getType(): The return type changed from 'java.lang.Class<java.lang.String>' to 'java.lang.Class<org.scijava.io.location.Location>'.
[ERROR] java.method.addedToInterface: method java.lang.Object org.scijava.io.IOService::open(org.scijava.io.location.Location) throws java.io.IOException: Method was added to an interface.
[ERROR] java.method.addedToInterface: method void org.scijava.io.IOService::save(java.lang.Object, org.scijava.io.location.Location) throws java.io.IOException: Method was added to an interface.
[ERROR] java.class.noLongerImplementsInterface: interface org.scijava.io.IOService: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.method.parameterTypeChanged: parameter void org.scijava.io.event.DataOpenedEvent::<init>(===org.scijava.io.location.Location===, java.lang.Object): The type of the parameter changed from 'java.lang.String' to 'org.scijava.io.location.Location'.
[ERROR] java.method.removed: method java.lang.String org.scijava.io.event.DataOpenedEvent::getSource(): Method was removed.
[ERROR] java.method.parameterTypeChanged: parameter void org.scijava.io.event.DataSavedEvent::<init>(===org.scijava.io.location.Location===, java.lang.Object): The type of the parameter changed from 'java.lang.String' to 'org.scijava.io.location.Location'.
[ERROR] java.method.removed: method java.lang.String org.scijava.io.event.DataSavedEvent::getDestination(): Method was removed.
[ERROR] java.method.parameterTypeChanged: parameter void org.scijava.io.event.IOEvent::<init>(===org.scijava.io.location.Location===, java.lang.Object): The type of the parameter changed from 'java.lang.String' to 'org.scijava.io.location.Location'.
[ERROR] java.method.removed: method java.lang.String org.scijava.io.event.IOEvent::getDescriptor(): Method was removed.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.script.io.ScriptIOPlugin: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.script.io.ScriptIOPlugin: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.script.io.ScriptIOPlugin: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.text.io.TextIOPlugin: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.text.io.TextIOPlugin: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.text.io.TextIOPlugin: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.

@imagejan
Copy link
Member

@frauzufall wrote:

I forgot how to include a test SciJavaPlugin without having to define a public class, can someone remind me?

Do you mean this?

https://github.com/scijava/scijava-table/blob/57e5472be079a7d65a89b2a9565fc94a227b8d05/src/test/java/org/scijava/table/io/TableIOServiceTest.java#L58-L60

@frauzufall
Copy link
Member Author

frauzufall commented Aug 12, 2020 •
edited
Loading

EDIT I made a separate PR #396 for the TypedIOService stuff.

@frauzufall frauzufall mentioned this pull request Aug 12, 2020
Merged
@frauzufall
Re-add reprecated methods
7d4a580 
* they were removed in favor of using Location instead of String for IO
handling
* they are re-added to restore backwards compatibility
@frauzufall
IOService: add default body to open(Location) and save(Object, Location)
d255cdd 
- this restores backwards-compatibility
@frauzufall
Copy link
Member Author

I fixed the most important API changes, this is the new status of revapi:

[ERROR] java.class.noLongerImplementsInterface: class org.scijava.io.AbstractIOPlugin<D extends java.lang.Object>: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.io.AbstractIOPlugin<D extends java.lang.Object>: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.io.AbstractIOPlugin<D extends java.lang.Object>: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.io.DefaultIOService: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.io.DefaultIOService: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.method.returnTypeTypeParametersChanged: method java.lang.Class<org.scijava.io.location.Location> org.scijava.io.IOPlugin<D>::getType(): The return type changed from 'java.lang.Class<java.lang.String>' to 'java.lang.Class<org.scijava.io.location.Location>'.
[ERROR] java.class.noLongerImplementsInterface: interface org.scijava.io.IOPlugin<D extends java.lang.Object>: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.method.returnTypeTypeParametersChanged: method java.lang.Class<org.scijava.io.location.Location> org.scijava.io.IOService::getType(): The return type changed from 'java.lang.Class<java.lang.String>' to 'java.lang.Class<org.scijava.io.location.Location>'.
[ERROR] java.class.noLongerImplementsInterface: interface org.scijava.io.IOService: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.script.io.ScriptIOPlugin: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.script.io.ScriptIOPlugin: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.script.io.ScriptIOPlugin: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.text.io.TextIOPlugin: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.text.io.TextIOPlugin: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.
[ERROR] java.class.noLongerImplementsInterface: class org.scijava.text.io.TextIOPlugin: Class no longer implements interface 'org.scijava.Typed<java.lang.String>'.

Not sure if there are more possibilities to fix any of this or if it would be desired at all.

I probably don't have anything to add to this PR until someone gives feedback.

This was referenced Aug 13, 2020
Merged
Merged
Merged
@ctrueden ctrueden merged commit 6ada478 into master Aug 13, 2020
@ctrueden ctrueden deleted the use-location branch August 13, 2020 16:35
@imagejan
Copy link
Member

imagejan commented Nov 1, 2020

@frauzufall wrote:

re-added the String methods to hopefully keep backwards compatibility.

Unfortunately, this still is a breaking change, as IOPlugin changes from HandlerPlugin<String> to HandlerPlugin<Location>, so existing IOPlugin implementations (as far as the extend AbstractIOPlugin) need at least be recompiled against this newer version of scijava-common (if I understand correctly).

See also #406.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@frauzufall @imagejan @imagesc-bot @ctrueden