-
Notifications
You must be signed in to change notification settings - Fork 4
Bump all generated-file and reviewed-file DFXML version references #45
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
Conversation
This patch clears TODOs that noted certain branches needed to merge in to the schema. Given the schema testing being used in file generation--parse round-tripping, all branches are assumed to have been merged. Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>
Absent feedback, I will merge this change Tuesday morning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend putting all of the version numbers in a single place, perhaps a file called constants.py
in a class called C
. Like this:
https://github.com/Plant-Tracer/webapp/blob/main/constants.py
it will make it easier to maintain...
@@ -43,7 +43,7 @@ def test_empty_file_object() -> None: | |||
|
|||
|
|||
def test_blank_file_object_filename() -> None: | |||
dobj = Objects.DFXMLObject(version="1.2.0") | |||
dobj = Objects.DFXMLObject(version="2.0.0-beta.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have all of these strings in a single location?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll put the value into /dfxml/__init__.py
, next to the XMLNS constants.
I think I'll also just have the version
keyword argument default to that constant string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want a single constant string... Should I wait off on merging this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please. I was mid-stream on something else, will handle now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is addressed now. If you have no other feedback, I'm fine with this being merged at your convenience.
Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay!
This PR clears TODOs that noted certain branches needed to merge in to the schema. Given the schema testing being used in file generation--parse round-tripping, all branches are assumed to have been merged.