fix: remove reserved keywords from kwargs before passing it to requests#117
fix: remove reserved keywords from kwargs before passing it to requests#117
Conversation
Codecov Report
@@ Coverage Diff @@
## main #117 +/- ##
=======================================
Coverage 98.77% 98.77%
=======================================
Files 18 18
Lines 732 737 +5
=======================================
+ Hits 723 728 +5
Misses 9 9
Continue to review full report at Codecov.
|
rmkeezer
left a comment
rmkeezer
left a comment
There was a problem hiding this comment.
This approach seems fine for now. We will need to make sure the reserved_keys is correct in the future for any changes made to prepared_request.
The pylint is failing right now for what it seems to be an unrelated "no-member" error that I believe is a false positive. I assume this was added in a new version of pylint. I would go ahead and disable "no-member" in the pylintrc file and include it in this PR.
Also, I think it would be beneficial to add a unit test to sanity check this code, but I'll go ahead and approve this now.
| # Remove the keys we set manually, don't let the user to overwrite these. | ||
| reserved_keys = ['method', 'url', 'headers', 'params', 'cookies'] | ||
| for key in reserved_keys: | ||
| kwargs.pop(key, None) |
There was a problem hiding this comment.
I wonder if we want to just throw an error here or a message so the user knows their argument is being omitted.
There was a problem hiding this comment.
I think throwing an exception is not a good idea, since removing the headers will be quite common. I've added a warning log for now.
padamstx
left a comment
padamstx
left a comment
There was a problem hiding this comment.
LGTM
After merging this PR, could you please do some additional testing with the platform-services python sdk (integration tests for a few services where you have the .env file, etc.)?
|
Sure! |
pyrooka
changed the title
pyrooka
changed the title
## [3.10.1](v3.10.0...v3.10.1) (2021-07-08) ### Bug Fixes * remove reserved keywords from kwargs before passing it to requests ([#117](#117)) ([6191978](6191978))
|
🎉 This PR is included in version 3.10.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
4 participants
This PR introduces a minor change/improvement: removal of some reserved keys from the
kwargsof the base services' send function.