-
Notifications
You must be signed in to change notification settings - Fork 270
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
api: Add Metadata.from_bytes() #1354
Conversation
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.
LGTM :)
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.
LGTM!
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.
LGTM. Minor question inline.
|
||
Arguments: | ||
data: metadata content as string. | ||
deserializer: Optional; A MetadataDeserializer instance that |
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.
is this notation standard? Would we be better off just relying on the typing annotations to indicate optional arguments?
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 saw that in a google style guide example -- could leave that out as well, it does duplicate the annotation...
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.
Let's leave it for now. Could you create an issue to ensure we review docstrings and make sure they are consistent before we cut a release.
Let's not merge just yet, I'm still going to doublecheck whether str is the correct type -- the Metadata API seems to use bytes in some places |
This is essentially short-hand for JSONDeserializer().deserialize(data) but seems much easier for the API user so may be worth it. Metadata.from_file() now uses Metadata.from_bytes() internally. Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
238264e
to
4e8738f
Compare
I had another look and decided
@joshuagl want to have another look? (mostly this was a literal replace of string with bytes) |
# Assert that it chokes correctly on an unknown metadata type | ||
bad_metadata_path = 'bad-metadata.json' | ||
bad_metadata = {'signed': {'_type': 'bad-metadata'}} | ||
bad_string = json.dumps(bad_metadata).encode('utf-8') |
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.
Is indentation off here, or is it just the GitHub app?
|
||
Arguments: | ||
data: metadata content as string. | ||
deserializer: Optional; A MetadataDeserializer instance that |
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.
Let's leave it for now. Could you create an issue to ensure we review docstrings and make sure they are consistent before we cut a release.
Looks good to me, thanks for the str->bytes change. |
GitHub Actions has encountered an internal error when running your job. Seems to be https://www.githubstatus.com/incidents/cj7gzzj30411 |
This is essentially short-hand for
but seems easier for the API user so may be worth it.
Metadata.from_file() now uses Metadata.from_bytes() internally.
(message and title edited later to replace string with bytes)
Fixes #1336
I think adding this makes sense in that we're trying to "hide" serialization details from the default use case and currently that wasn't possible if you just have a string (and not a file).
Please verify and check that the pull request fulfills the following
requirements: