-
Notifications
You must be signed in to change notification settings - Fork 949
Change upload value to contain Python types #2767
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
Change upload value to contain Python types #2767
Conversation
This was never used. A single global error is used instead.
c5b5609
to
ddc9b33
Compare
This is now ready for review. |
ipywidgets/widgets/widget_upload.py
Outdated
js = {} | ||
for attribute in ['name', 'type', 'size', 'content']: | ||
js[attribute] = uploaded_file[attribute] | ||
js['lastModified'] = int(uploaded_file['last_modified'].timestamp() * 1000) |
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.
Our general convention, at least in default controls, has been snake_case for the attributes in the protocol messages. Should we keep it snake_case here, as last_modified
?
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 option is to just call it modified
, or even mtime
, which mirrors the stat()
structure and how you address it in python and presumably lots of other languages that let you make a stat call.
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.
mtime
in Python is a timestamp, whereas we're returning a datetime. That warrants using a different name, I think.
I'll go withlast_modified
, but this is not strongly held (so feel free to change it).
This looks great! A couple of inline comments above, and then I think it's ready to go. |
Thanks for the feedback. I made all the changes discussed. |
Thanks! This looks really great. |
Prior to this PR, the
.value
traitlet for theFileUpload
widget was a list of dictionaries. Each dictionary contained the raw metadata generated by the browser, as well as the content as a memoryview.This PR changes the return value to be a tuple of
Bunch
instances. Attribute names inside the bunch are converted to snake case and the last modified is converted to a zoned datetime.For instance:
This PR also removes syncing an 'error' attribute per file. This was not exposed in the kernel, and it was never set to anything apart from an empty string in the frontend. Note that the global error attribute remains.
Note that this PR has some overlap with #2715 : both share datetime serialization / deserialization considerations. In here, I have done the least amount of work -- I just send a timestamp over the wire, and manage conversion in the kernel. Once 2548 is merged, it may be worth overhauling serialization to have a common interface. That won't be a breaking change.
Closes issue #2727 .