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

Modulo retrieve #576

Closed
wants to merge 11 commits into from
Closed

Modulo retrieve #576

wants to merge 11 commits into from

Conversation

jburel
Copy link
Member

@jburel jburel commented Jan 3, 2013

Add method to load given type of annotations linked to objects.
see https://trac.openmicroscopy.org.uk/ome/ticket/10043

In this PR:

  • Add a new method.
  • Add support for modulo NS.
  • Add tests.

To test the PR, run

./build.py -f components/tools/OmeroJava/build.xml test -DTEST=integration/MetadataServiceTest

and (for missing XmlAnnotation support)

./build.py -f components/tools/OmeroJava/build.xml test -DTEST=integration/UpdateServiceTest

@joshmoore
Copy link
Member

@jburel, what's the argument for putting this on 4.4? I'm a bit worried about what this does to client/server interactions between new and old versions.

@jburel
Copy link
Member Author

jburel commented Jan 4, 2013

@joshmoore: I do not have the intention to support it in our client. Ian needs to move forward in his support for FLIM.
Basically he can quickly retrieve the modulo file linked to an image and parse it.

@joshmoore
Copy link
Member

Sure, but regardless of whether or not it's in the clients, it still has an impact on the generated code. Let's at least be sure we test what happens between different versions before merging.

@jburel
Copy link
Member Author

jburel commented Jan 4, 2013

@joshmoore: of course. It is a new method, existing methods have not be modified.

@qidane
Copy link
Contributor

qidane commented Jan 4, 2013

All tests passed

@qidane
Copy link
Contributor

qidane commented Jan 4, 2013

Changes look ok.
Ready to merge.

@jburel
Copy link
Member Author

jburel commented Jan 5, 2013

@imunro: Is the newly added method what you were expected (cf.discussion before christmas)?

@chris-allan
Copy link
Member

Considering the generic nature of the method and its potential to be used by any, if not all, clients perhaps we could be a bit more thorough with the documentation? At least trying to be as thorough as loadAnnotations() would be a great start which has an almost identical signature.

@imunro
Copy link
Contributor

imunro commented Jan 8, 2013

It does look as if it will do exctly what we want thanks guys. I should say that this isn't holding us up. It is possible to get the same info with existing methods although it's a bit long winded.

@jburel
Copy link
Member Author

jburel commented Jan 8, 2013

@imunro, @joshmoore: I misunderstood. I thought it was a blocker. in that case I might only add the method to develop and not to a point release.

@jburel
Copy link
Member Author

jburel commented Jan 9, 2013

@joshmoore, @imunro: Discussed with Ian, easier to do against develop. J-M to help Ian to use Iquery in the meantime if required.
This PR is replaced by #587

@jburel jburel closed this Jan 9, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants