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

add enumeration values for acquisition mode #2553

Merged
merged 5 commits into from
Sep 21, 2016
Merged

Conversation

mtbc
Copy link
Member

@mtbc mtbc commented Sep 1, 2016

Increments OME-XML schema version attribute value and adds acquisition modes,

  • BrightField
  • SweptFieldConfocal
  • SPIM

and parsing for,

  • Laser Scan Confocal
  • Swept Field Confocal

The schema namespace is left unchanged.

@rleigh-codelibre
Copy link
Contributor

Looks good to me in principle.

However, do we want to add new enumerations to the end of the list to preserve ABI? We don't have a fixed ABI for C++, so probably not too important there at this point in time, but it would break the ABI by changing the enum values. How do enum additions affect Java? Not necessarily something which needs affect this PR, but something to bear in mind for how we handle ABI changes in the future.

@sbesson
Copy link
Member

sbesson commented Sep 6, 2016

@rleigh-codelibre thanks for raising the ABI considerations and agreed that we might want to investigate how to minimize breakages in the future - see https://ci.openmicroscopy.org/view/Failing/job/BIOFORMATS-DEV-breaking-generated-sources/lastSuccessfulBuild/ for the current enum generated changes on C++ and Java.

@rleigh-codelibre
Copy link
Contributor

The latest changes look fine, and the generated source changes show all enums are appended which will preserve the existing enum values.

This looks good to merge, thanks.

@hflynn
Copy link
Member

hflynn commented Sep 21, 2016

Will this require updates to the Model docs too @mtbc ?

@mtbc
Copy link
Member Author

mtbc commented Sep 21, 2016

Just the autogenerated really, though it's probably also worth a line in "what's new"; we'll catch that in the routine diffs I suggested.

As for the more general issue of extending enumerations, probably the docs checklist item on https://trello.com/c/0zNp8HvU/167-enumerations-addition will cover it, @sbesson?

@sbesson
Copy link
Member

sbesson commented Sep 21, 2016

Documentating the changes in the version 2 of the 2016-06 schema certainly sounds reasonable, I'll add a note to the checklist. Otherwise all tests are passing including the integration testing at the OMERO level which auto-detects new enumeration. Parsing the logs of the [last full repository job](https://ci.openmicroscopy.org/job/BIOFORMATS-DEV-merge-full-repository/293/ for improperly parsed values shows that the new enumeration are properly picked up.

sbesson@lifesci-51950:target $ grep WARN.*Unknown * | wc -l
  171504
sbesson@lifesci-51950:target $ grep WARN.*Acquisition * | wc -l
       0

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

4 participants