search

imagej / imagej-server

A RESTful web server for ImageJ [EXPERIMENTAL]
Apache License 2.0
38 stars 17 forks source link

Enable interval serialization #30

Closed PetrBainar closed 4 years ago

maarzt commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

PetrBainar commented 5 years ago

Suggestion #4 has been addressed by creating an issue (https://github.com/imagej/imagej-server/issues/31)

PetrBainar commented 5 years ago

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

Addressed

PetrBainar commented 5 years ago

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