-
Notifications
You must be signed in to change notification settings - Fork 506
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 .with_headers
to support multiple k/v pairs
#2699
Fix .with_headers
to support multiple k/v pairs
#2699
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2699 +/- ##
=======================================
- Coverage 79.3% 79.3% -0.1%
=======================================
Files 123 123
Lines 22657 22660 +3
=======================================
+ Hits 17975 17977 +2
- Misses 4682 4683 +1 ☔ View full report in Codecov by Sentry. |
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.
Thanks! Could you add a one-line to the changelog for this bug fix.
31e5439
to
2795f2f
Compare
@cijothomas done. Not sure what the integration tests failure is about, though. Maybe a fluke and worth retrying? |
Changes
Currently,
.with_headers
doesn't work consistently when passing in multiple k/v pairs. For instance, something like this only adds one of the headers:This is quite surprising -- only one header is actually accepted, and it's a random choice. The others are lost. To work around this with the existing code, add one k/v at a time like this:
I've updated the tests to cover the surprising scenario, and adjusted the logic to support multiple k/v pairs.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes