-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix typing errors on content_disposition_header() #8698
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8698 +/- ##
==========================================
+ Coverage 97.69% 97.96% +0.27%
==========================================
Files 108 107 -1
Lines 34075 33861 -214
Branches 4055 3970 -85
==========================================
- Hits 33288 33171 -117
+ Misses 587 513 -74
+ Partials 200 177 -23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Maybe this should get a changelog message as a breaking change It does look like its used externally https://github.com/Starry-OvO/aiotieba/blob/1018eb96f26e6c17b7503f3457ad0accc2a9eb35/aiotieba/core/http.py#L135 some more on github code search https://github.com/search?q=content_disposition_header+aiohttp&type=code&p=2 |
Most of the results are just forks of aiohttp. I can see one which already passes params as a dict in the second argument, which wouldn't even work with the old code... |
The signature for the function doesn't type check well. If we unpack a
dict[str, str], we'll get a type error because quote_fields requires a bool and could be included in the unpacked arguments. All locations using the function seem to have a dict anyway...This appears to be an internal function, not documented in our API, so risk should be low by changing the signature. Though will only do it on master anyway.