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

Fixes to centimeterPerYear, millimeterPerDay, and molePerHectare #294

Closed
wants to merge 1 commit into from

Conversation

@maier-m
Copy link
Contributor

maier-m commented Apr 3, 2018

No description provided.

@mbjones

This comment has been minimized.

Copy link
Contributor

mbjones commented Apr 3, 2018

Thanks, @maier-m -- appreciate this. My only comment is I wonder if the removal of the whitespace on millimeterPerDay is backwards compatible. I'd like to see a test document added to tests that uses the standardUnit with the space like <standardUnit>millimeterPerDay </standardUnit> and then verify that the document still validates under the new schema. Could you make those changes and we'll see how the validation tests go in Travis? Note that there is also an enumeration of standardUnits in eml-unitTypeDefinitions.xsd that should be updated as well for any name changes.

@mbjones mbjones added the bug label Apr 3, 2018
@mbjones mbjones added this to the EML2.2.0 milestone Apr 3, 2018
@maier-m

This comment has been minimized.

Copy link
Contributor Author

maier-m commented Apr 3, 2018

OK.

  • I think millimeterPerDay is a new unit and so shouldn't cause problems with backward compatibility?
  • It doesn't look like the eml-unitTypeDefinitions.xsd enumeration was updated with the addition of units in the previous commit. Should I update with those units as well?
  • I'm sorry I am a little unclear on how to incorporate a validation test in this repo. Can you point me to an example?
@maier-m maier-m force-pushed the maier-m:master branch from dcc5461 to 9370189 Apr 3, 2018
@mbjones mbjones added the in progress label Apr 3, 2018
@mbjones

This comment has been minimized.

Copy link
Contributor

mbjones commented Apr 3, 2018

OK, if its a new unit then fixing the spaces makes sense. But yes, we should go ahead and add all of the units to the enumeration.

For testing, we check that all of the xml files in src/test/resources/ are schema valid and pass the EMLParser checks. So, adding the use of the unit to one of the test files in that directory will tell you whether it validates or not. You can run those tests locally on your machine before you commit by running ant test in the root of the EML directory (assuming you have the required software installed).

@maier-m

This comment has been minimized.

Copy link
Contributor Author

maier-m commented May 16, 2018

these should now be covered in #295

@maier-m maier-m closed this May 16, 2018
@gothub gothub removed the in progress label May 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.