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

Enable interval serialization #30

Merged
merged 15 commits into from
Jul 9, 2020

Conversation

PetrBainar
Copy link
Contributor

No description provided.

@maarzt
Copy link
Member

maarzt commented Feb 26, 2019

The PR should be improved:

  1. ObjectMapperModifier should be a interface not an abstract class.
  2. Instead of ObjectMapperModifier I would like to have more meaningful interfaces like JsonSerializer and JsonDeserializer. Both should have a nice easy to implement interface.
  3. An example should be included like maybe serializer and deserializer for Interval.
  4. JsonService should become a real SciJavaService.

@ctrueden
Copy link
Member

ctrueden commented Feb 26, 2019

Thanks for reviewing, @maarzt!

About the JsonService and other non-SciJava services: I think part of the confusion is that the Dropwizard stack has its own service model, so the term "service" becomes technically ambiguous in this project. And further adding to this confusion: I don't think the JsonService is a Dropwizard service either! 😆

It would be nice if someone who takes the time to understand both architectures (SciJava and Dropwizard) could clean up this project. But I don't think JsonService not being a SciJava service is a blocker for this PR.

@kozusznik kozusznik force-pushed the preparingForMergeRequest branch from 9ca720d to 6d3d3df Compare February 27, 2019 10:04
@kozusznik
Copy link
Contributor

I have incorporated suggestions 1 and 2 - it is commited in this branch.

@PetrBainar
Copy link
Contributor Author

Suggestion #4 has been addressed by creating an issue (#31)

@PetrBainar
Copy link
Contributor Author

Consider using interface type Map instead of class.
Consider using HashMap instead of LinkedHashMap

Addressed

@PetrBainar
Copy link
Contributor Author

I think that all suggestions have been addressed. @maarzt and @ctrueden do you guys think we can carry on and merge? Thank you!

@kozusznik kozusznik force-pushed the preparingForMergeRequest branch from 7c8c38c to 967f973 Compare May 9, 2019 08:18
@ctrueden ctrueden changed the title WIP: Enable interval serialization Enable interval serialization Jul 9, 2020
@ctrueden ctrueden merged commit d69f215 into imagej:master Jul 9, 2020
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.

4 participants