search

ome / ome-model

OME model (specification, code generator, implementation)
Other
13 stars 26 forks source link

Fix cyclic dependency between specification and ome-xml #101

Closed sbesson closed 5 years ago

sbesson commented 5 years ago

The source generation of ome-xml depends on the XSD resources in the specification component but some specification Java classes depend on the generated ome.xml classes. This commit works around this cyclic dependency by migrating the XML classes to the ome-xml component and having ome-xml depending on specification.

From a consumer perspective, the impact should be fairly minimal. The package name is kept unchanged and the ome-xml already needs to be declared as a dependency to import e.g ome.specification.XMLMockObjects.

rleigh-codelibre commented 5 years ago

Could the moved java files in specification be moved directly where they really belong: alongside the consumers in Bio-Formats? They shouldn't really be in either component in this repository. If they can't be moved to Bio-Formats, could they go into a third component in this repository, which Bio-Formats can then depend upon?

sbesson commented 5 years ago

Hi @rleigh-codelibre, thanks for the feedback. Given timelines and other priorities, the scope of this PR was limited to fixing the build dependency issue while remaining compatible with a minimal patch/minor release increment.

As the classes primarily consume ome.xml.* classes to construct mock XML objects, keeping them alongside the generated object classes in this repository felt like a natural place. We can definitely start the discussion about the name, namespace and intent of a new component and deprecate these classes if we agree with the migration. Other reviewers might have additional thoughts on this.

dgault commented 5 years ago

The 2 files moved here do appear to be the only ones affected so this does solve the immediate issue of the dependency loop. The only places in the rest of the code base that would be affected are FakeReader and the schema tests. With the package name unchanged and ome-xmlalready as a dependency there is no negative impact here.

In the longer term I agree that these don't inherently fit into this component or indeed Bio-Formts, but an additional component would seem to me like complete overkill in this instance. I would be happy with them living with ome-xml.

sbesson commented 5 years ago

Thanks. Merging for a quick patch release.

We can keep driving the discussion about the migration to a new component. As an extra piece of information, the other place in the OME stack consuming these objects are the OMERO.java integrationt tests.

jburel commented 5 years ago

As indicated by @sbesson, XMLWriter and mock ones are using in the importer tests so we will see if we any impact/adjustment required