Skip to content
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

Consolidate the file upload widget value metadata and content? #2721

Closed
jasongrout opened this issue Jan 11, 2020 · 5 comments
Closed

Consolidate the file upload widget value metadata and content? #2721

jasongrout opened this issue Jan 11, 2020 · 5 comments
Labels
resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Milestone

Comments

@jasongrout
Copy link
Member

jasongrout commented Jan 11, 2020

We've recently discussed the format for the file upload value attribute. It looks like what was settled on was something like:

[{'metadata': {'name': 'Untitled1.ipynb',
   'type': '',
   'size': 1734,
   'lastModified': 1552316652863},
  'content': <memory at 0x10eeadd00>}]

In playing around with it a bit, it seems much simpler to just combine it into one dictionary, sort of like how the Jupyter contents api does it:

[{'name': 'Untitled1.ipynb',
   'type': '',
   'size': 1734,
   'lastModified': 1552316652863,
   'content': <memory at 0x10eeadd00>}]

I think it's simpler to not have to go through the metadata key, like:

for f in widget.value:
    print(f['name'])
    print(f['size'])
    print(f["content"].tobytes().decode("utf-8"))

Thoughts?

@jasongrout jasongrout added this to the 8.0 milestone Jan 11, 2020
@pbugnion
Copy link
Member

I agree having a single top-level dictionary is simpler. I think this was just forgotten about when we pushed #2666.

@jasongrout
Copy link
Member Author

Also, I notice in the documentation from #2714, we have an example of converting the contents to a string: uploaded_file["content"].tobytes().decode("utf-8"). As far as I can tell, this creates an extra copy (tobytes does a copy). It would probably be better to do:

import codecs
codec.decode(uploaded_file["content"], 'utf-8')

which does not do an extra copy by working on the memory view directly.

@pbugnion
Copy link
Member

pbugnion commented Jan 11, 2020

Ah I hadn't realised that difference between .tobytes and using codecs. Thanks!

I've addressed the docs issue in #2722.

@jtpio
Copy link
Member

jtpio commented Jan 11, 2020

Agree a single dict is simpler.

PR incoming.

@pbugnion
Copy link
Member

Fixed by #2724 and #2722.

@lock lock bot added the resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label May 20, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

No branches or pull requests

3 participants