-
Notifications
You must be signed in to change notification settings - Fork 37
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
Files arg #189
Files arg #189
Conversation
@FoamyGuy is this ready for final review or is it still in progress/ |
Files arg plus foamyguy
I believe this is ready for review now. I've merged Justin's changes that were submitted against my branch to this so they are included here now. |
@dhalbert I created a new PR into his branch to add some tests back in. Would love to see those in (can add later if wanted) |
…d timeouts to python_requests.post.
@dhalbert my tests are merged and this is ready for review! |
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.
A few suggestions.
@FoamyGuy shall I make another PR into this branch with the changes? |
I got the change to os.urandom() with the latest commit, if you've got a clear idea how to tackle reducing the string concatenation then definitely feel free to PR to this branch. I'll try to take a crack at it over the weekend if you don't have the opportunity to get it first. |
@FoamyGuy created here: FoamyGuy#4 |
Remove str concatenation
@dhalbert ready to go again! |
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.
There are more AttributeError
usages that what I marked. They are in older code that isn't touched by this PR. Could you change those as well? Thanks. The tests will also need to be changed to use ValueError
.
I looked at the bundle and there are many libraries that use AttributeError
in places where should use ValueError
. Maybe I should bring this up in the Monday meeting. There are also some references in Learn code.
This will be a major version change due to the exception changes.
Maybe the |
@dhalbert I'm happy to work through this repo and fix up the exceptions and tests, and can do so after this is merged if that is the only concern. |
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 am willing to add this -- I am a little worried about library size but we'll see if it causes issues when frozen.
@dhalbert I had this idea of a |
@FoamyGuy Thanks for merging. This is an upward-compatible improvement, so it deserved a minor version bump. But no need to change the version number now. |
Updating https://github.com/adafruit/Adafruit_CircuitPython_Requests to 3.2.10 from 3.2.9: > Merge pull request adafruit/Adafruit_CircuitPython_Requests#189 from FoamyGuy/files_arg Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Updated download stats for the libraries
This change adds a
files
argument that can be passed topost()
The value supported is dictionary with string keys and tuple values. The behavior when passed matches the behavior of CPython requests.
A new example was added illustrating the functionality, I've only tested with native wifi on ESP32 S3, so that is the only one added.
I was able to confirm the upload is working correctly by using a local simple flask server that acceps file upload and by examining the result returned by httpbin.org when the new example runs. It returns the image back in base64 format. This cyberchef recipe was used to validate that the original image does pop back out the other side https://cyberchef.org/#recipe=From_Base64('A-Za-z0-9%2B/%3D',true,false)Render_Image('Raw')
Note that the CPYthon implementation of this also supports a
files
dictionary with string keys and file_handle values (as opposed to tupple containing the file_handle and other things). That form of the argument is not supported on circuitpython. In order for that to be supported I believe we would need access to FileIO.name property or something that provides similar functionality. I've created #9195 in the core inquiring about adding that, but at present time this is not supported in the circuitpython version.