Fix #1053 by changing request body format to params #1054
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
seratch
added
enhancement
seratch
commented
Jul 1, 2021
| @@ -26,7 +26,7 @@ async def setUp(self): | |||
| ].replace("#", "") | |||
| client = AsyncWebClient(token=token) | |||
| self.channel_id = None | |||
| async for resp in await client.conversations_list(limit=10): | |||
| async for resp in await client.conversations_list(limit=1000): | |||
There was a problem hiding this comment.
To avoid ratelimited errors with my test workspace (it has a very large number of channels)
| @@ -621,7 +621,7 @@ async def admin_users_session_getSettings( | |||
| kwargs.update({"user_ids": ",".join(user_ids)}) | |||
There was a problem hiding this comment.
This code currently works but the server-side may stop accepting this format in the future. That's the reason why we want to make the changes in this PR.
Codecov Report
@@ Coverage Diff @@
## main #1054 +/- ##
=======================================
Coverage 84.36% 84.36%
=======================================
Files 95 95
Lines 8937 8937
=======================================
Hits 7540 7540
Misses 1397 1397
Continue to review full report at Codecov.
|
misscoded
approved these changes
Jul 1, 2021
|
Thanks for your prompt review! |
16 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This pull request fixes #1053 by eliminating the potential issues with a single comma-separated-value format data for a sequence type parameter (e.g., channel_ids, user_ids, etc.) in JSON request body.
As far as I did tests with the mentioned endpoint, a single CSV format string value works even in the JSON request body. That being said, it should be much safer to switch the content-type from
application/jsontoapplication/x-www-form-urlencodedin the long run. It is a proven way in the Java SDK and some others.We may want to eventually switch all the API methods to
application/x-www-form-urlencodedfor simplicity but it's out of this pull request's scope.Category (place an
xin each of the[ ])/docs-src(Documents, have you run./docs.sh?)/docs-src-v2(Documents, have you run./docs-v2.sh?)/tutorial(PythOnBoardingBot tutorial)tests/integration_tests(Automated tests for this library)Requirements (place an
xin each[ ])python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.