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
Add default user agent header #1750
Conversation
Well, that travis build clearly isn't good. Let me look into that tomorrow |
Hopefully that one does it; lacking access to a non-windows machine at the moment, which is my guess for why I'm seeing tens of inconsistent failures locally. |
Codecov Report
@@ Coverage Diff @@
## master #1750 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 23 23
Lines 2029 2043 +14
=========================================
+ Hits 2029 2043 +14
Continue to review full report at Codecov.
|
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 for opening this PR! 🎉 I have a few comments before it can be merged.
Whelp, I'll figure that one out tomorrow. |
I think the mysteries of the app engine is a story for another day. |
@DonaCthulhuote We do follow your progress with attention! And are happy to help out if you're stuck. |
Oh, just realized what that comment sounded like; shouldn't write things other people can see when I'm tired. I simply thought it was funny that the context for my statements was disappearing. And not stuck (yet); just discovering that writing code for multiple python versions is a skill in and of itself. |
I'm certain the tests will all pass now. Prove me wrong, travis! |
Just a quick note to mention that the remaining failures here are not in your code! So if you think this is ready for review, then it is ready indeed :) |
Have at it, if you will. |
@DonaCthulhuote Sorry I'm going to ask one more thing first -- can you please rebase your commit on top of master? We only merge pull requests that pass CI, and the only way to do that here is to rebase. |
I'm hoping those should still be green; my local for some reason decided it'd have trouble running tests after rebasing. If travis discovers something, I'll take a deeper look tonight. |
@DonaCthulhuote we need to review this, sorry that we haven't be able to do it yet. You no longer need to rebase your commit on top of master since the tests pass and there are no conflicts! |
I've rebased the work @DonaCthulhuote did on top of master. I still need to figure out where and how to document the new user agent choosing logic (default and suppressing). Regarding @user234683's comment, I'm also leaning towards removing the version from the user agent. @sethmlarson @pquentin your thoughts? |
I'd use None for the suppress headers, if it's being added only to be removed, but I don't have a good justification other than that it feels cleaner in my head. Thanks for picking this up; I got run over by work last week, and hadn't had a chance to take another look at this. |
@DonaCthulhuote You’re right, it is cleaner and it was my first attempt. I’ve reverted it because it didn’t work with The same problem will probably rise for higher-level APIs (i.e. requests) that will expose user agent as an optional argument. What do you think? |
I think I clearly missed that case, and that you've clearly put more thought into the problem then I have :) . I'll defer to you and the maintainers on the matter. |
I don't have a strong opinion here. requests sends @hodbn What's the problem with |
My bad 🙃 not urllib3/src/urllib3/util/request.py Lines 22 to 29 in 4901ec7
Callers will not be able to suppress the default behavior. I guess there might be other callers that will have the same problem. I'm not that happy with the constant I've introduced, do you think a sentinel object will be cleaner? |
Co-authored-by: hodbn <hodbn@users.noreply.github.com>
For #1296
No tests fail that didn't already fail when I ran them locally (which isn't all that hot, I know; not sure what's going on there)
Should I try to add this behaviour to the docs as well, or it okay being implicit?