OME/@Creator

[ngladitz]

This is meant to add broader support for the OME/@Creator attribute. Specifically to fix issue #103

I primarily tried to duplicate the existing logic I found for OME/@UUID. I am not sure I understand every context where I applied these changes and I am not sure I found every relevant context.

A such I would appreciate an appropriately critical review of these changes.

[rleigh-codelibre]

@ngladitz The changes all look reasonable, with one caveat. Other than the MetadataConverter changes, all the rest of the MetadataStore/Retrieve changes should have been automatically code generated with xsd-fu without the need to hardcode changes in all of the templates. I don't understand (without deeper digging) why the Creator attribute is special in this regard. However, there are other hardcoded additions in the templates already, and it might be that there's an underlying cause which underlies this code generation defect.

None of the above is to suggest that the change should not be merged as is, since there's precedent for this type of change, but we might want to look at investigating and fixing some defects in xsd-fu.

[rleigh-codelibre]

Regarding the C++ template and MetadataConverter changes, there are problems here. Please see the GitLab build pipeline for more details.

[rleigh-codelibre]

@ngladitz In addition to the comments above, please see the last two commits on GitLab for the needed fixes, and this GitLab pipeline for the passing builds and generated source changes diffs for both C++ and Java. Both look fine other than the minor pointer dereference I mentioned above for C++.

@sbesson Are there any plans to re-establish C++ CI testing by OME? Without it, the chance of regressions is very high. If you don't want to keep it around, I could open up a PR to drop it if that is preferable. Or you can pull the ome-common artefacts like these for use in your CI, or use GitLab runners similar to what I'm using. I'm happy to share the setup instructions.

[ngladitz]

@rleigh-codelibre thank you! I hope I fixed all the issues you found (or rather correctly incorporated your fixes).

[rleigh-codelibre]

@ngladitz Thanks! There's one missing change remaining: OMEXMLMetadata conditional, tested in this pipeline.

[ngladitz]

Ah sorry I hope I got them all this time; thanks again!

[ngladitz]

@sbesson I've pushed two new commits; one that actually bumps the java version from 7 to 8 and one that adds the default functions to the java interfaces (and removes the now redundant implementation from DummyMetadata). Assuming this is what you expected I could squash the default method commit and put the version bump commit before it.

[rleigh-codelibre]

@ngladitz Your latest changes pass without any problems: GitLab pipeline.

The Java changes all look fine as well.

I do wonder with the change to Java 8 if we could (as a future improvement) create similar stubs for all MetadataStore and MetadataRetrieve interfaces, leaving DummyMetadata as an empty class extending that interface.

[ngladitz]

Hi thanks again ... @sbesson I think I might have missed and due to force pushing lost the explicit changes you requested. Do the current changes encompass what you proposed?

[sbesson]

Thanks @ngladitz, yes the default methods implemented in 848e76dee15397d9eb1c1dda15c8c1bee2452de6 correspond to what I had in mind. I can confirm they are sufficient to avoid implementations of the MetadataStore interface outside this component to break and make this change amenable for a minor release increment.

I spend some time trying to construct a minimal example exercising these new Java interfaces for example when converting a file in Bio-Formats. However, I still need to work around some of the downstream code which is overriding the OME@Creator. I will keep working towards an example validating the proposed change early next week so that we can be in a position to merge this PR.

[sbesson]

Thank you @ngladitz. Merging after a final sign-off from the @ome/formats and we will work towards enabling this in the upcoming Bio-Formats minor release.

[ngladitz]

Thanks!