-
Notifications
You must be signed in to change notification settings - Fork 2k
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
updating metadata with multiple uris fix #11538
Conversation
key = metadata_entry[0] | ||
if key == b"u": | ||
new_metadata.append((b"u", [condition.rest().rest().first().atom] + list(metadata_entry[1:]))) | ||
else: |
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 it reasonable to assume that h
is the only other metadata key? I thought we were going to support keys like l
, lh
, m
and mh
(or similar), but maybe those can't be updated at this point.
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, only made a fix for current codebase, this will need to be expanded as part of the "support updating metadata url and license" task
for metadata_entry in metadata_list: | ||
key = metadata_entry[0] | ||
if key == b"u": | ||
new_metadata.append((b"u", [condition.rest().rest().first().atom] + list(metadata_entry[1:]))) |
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.
Why list(metadata_entry[1:]) here? Shouldn't be metadata_entry[1]? Since it's a list of uris
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.
metadata_entry looks something like this when deserialized: (b'u', 'url1', 'url2' ... )
not sure why .as_python
deserializes it like 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.
lgtm
No description provided.