Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Move metadata class model de/serialization to sub-package #1279
Move metadata class model de/serialization to sub-package #1279
Changes from 1 commit
3d8cade
4a22b4a
499f1c8
240fb54
e1be085
8e9afc9
2f57eb8
2b40857
aa8225c
d823c8f
aba6ba3
f8fc5e2
ace25e4
326d2af
ab92ba2
bd94f6d
a53d68b
ef91964
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Note: the recently adopted style guide recommends against importing individual classes or functions.
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 know. I guess I just kept using the convention that we already used in this file, which we started before fully embracing our new style guide... and because PEP 8 says it's okay.
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 agree with Joshua on this one.
We don't have a linter that would automatically catch those mistakes, but it's important we correct each other.
Also, it seems logical to forbid importing of functions and classes.
This way we prevent future problems with naming and namespaces.
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.
Nit: could we rename this variable, rather than using the
_
trick? Maybe json_dict, json_object (as the return type ofjson.loads
is a Python object), or loaded_json.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.
Do you think it is worth clarifying, via the class name, that this is the OLPC canonical JSON?
You mention it in the header above, but as we have had several adopters surprised that our Canonical JSON isn't compatible with go-tuf's Canonical JSON, I wonder whether it is worth being very explicit?
Counter-argument, I do not like
OLPCCanonicalJSONSerializer
as a class name...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 agree with both arguments. :D
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.
Another way to avoid ambiguity would be to not call it
CanonicalJSONSerializer
at all, but something likeDefaultSignedSerializer
or so? Then people are forced to read the docstring to get more info. At any rate, I will mention OLPC in the class and function docstrings.