Skip to content

Conversation

@ryanhiebert
Copy link
Contributor

Fixes #278

I haven't added any tests, but I can later.

@Lukasa
Copy link
Member

Lukasa commented May 4, 2017

So, this definitely looks plausible. I think we need some testing: in particular, I think we need some testing about how this interacts with the other places we process auth (see anywhere that has auth= in the codebase).

@ryanhiebert
Copy link
Contributor Author

I'll try to put in some more time this evening to figuring out the testing.

@singingwolfboy
Copy link
Member

Hey @ryanhiebert, what's the status of this pull request?

@coveralls
Copy link

coveralls commented Jul 13, 2018

Coverage Status

Coverage increased (+0.03%) to 88.05% when pulling 0c9287f on ryanhiebert:noop-auth into 4fb0db5 on requests:master.

@ryanhiebert
Copy link
Contributor Author

Thank you for the nudge. This latest commit fixed the bugs, but I still haven't added any regression testing.

@ryanhiebert
Copy link
Contributor Author

I am working on the regression test, but looking at @Lukasa 's comment to me about checking on auth that we do, I'm not sure how to do any of that. My impression is that there's nothing really to test with that, because all of the current tests should handle those cases.

The only thing I'm seeing being worth adding AFA tests is the regression test to make sure that the .netrc isn't used, even though thats the default for requests.

@ryanhiebert
Copy link
Contributor Author

ryanhiebert commented Jul 13, 2018

Digging into the session code of requests a bit more, it looks like there's a trust_env setting that defaults to True.

http://docs.python-requests.org/en/master/api/#requests.Session.trust_env

Would settings trust_env to False be a better option than the noop auth? It'd certainly work for my use-case. The downside is that it looks like it also bypasses any proxies, so it's not strictly relegated to just avoiding using the .netrc files, though I'm not sure how much of an issue that caveat is.

@ryanhiebert
Copy link
Contributor Author

ryanhiebert commented Jul 13, 2018

OK, I've added a regression test suite, that inherits from the main test suite but sets up a problematic .netrc file before it begins, and caused 2 of the tests to fail (so they are good regression tests). I've also added an entry in the HISTORY file.

I've kept the no-op auth as the approach, because I'm a bit hesitant to incur the side-effect of bypassing proxies, and because the no-op lambda is already short enough to make me happy.

@singingwolfboy
Copy link
Member

Looks great to me! @Lukasa, what do you think?

@ryanhiebert
Copy link
Contributor Author

What are we thinking about this?

@singingwolfboy
Copy link
Member

@ryanhiebert if you rebase to resolve the conflicts, and the tests still pass, then I'll merge this.

@ryanhiebert
Copy link
Contributor Author

Thanks, @singingwolfboy . I've done that.

@singingwolfboy singingwolfboy merged commit e859dbd into requests:master Jan 13, 2019
@ryanhiebert ryanhiebert deleted the noop-auth branch January 13, 2019 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants