Fix regression in API caused by commit 85400d8d6751071ef78f042d1efa72…

[hile]

…bdcf76cc0e

• The added optional parameter changes API and should default to None

  This utility call is used by for example requestbuilder package directly
  which breaks because it passes only one argument to the function as it
  used to be.

[kennethreitz]

I don't believe this is a public API, and therefore we don't gaurentee API compatibility.

But, I could be wrong — either way, it seems like a nice change to make if it doesn't hurt anything.

@nateprewitt any objections?

[Lukasa]

I think this is a good idea, but we should work out why the builders are failing first. ;)

[nateprewitt]

Yeah, I don't see any issue with this, even if we're not supporting this API publicly.

As for the build, something in our dependency pipeline must have changed yesterday or earlier. Python <= 3.3 builds started failing on test_https_warnings, but I wasn't able to determine why at a quick glance. @Lukasa probably has more background about the architecture underneath this.

[Lukasa]

I think this has to be a pytest-httpbin thing. The error is ultimately because we now believe that the cert that pytest-httpbin is presenting on the secure endpoint now has a subjectAlternativeName field. I'm trying to locate it and work out why that warning isn't firing.

[Lukasa]

Hrm, no, that's not true. This test passes if run by itself, so this is a new state based test failure for some reason.

[Lukasa]

Ok I have a theory, testing now.

[nateprewitt]

The new release of Pytest from yesterday has some interesting notes in the changelog.

• The pytest-warnings plugin has been integrated into the core, so now pytest automatically captures and displays warnings at the end of the test session. Thanks @nicoddemus for the PR.

• pytest.warns now checks for subclass relationship rather than class equality. Thanks @lesteve for the PR (Requests support HTTPS proxies timeout #2166)

[Lukasa]

Yeah, this is the warning issue: see #4056.

[Lukasa]

Ok cool, can we rebase this on the new master?

[kennethreitz]

@Lukasa no conflicts!

[nateprewitt]

@kennethreitz, I kicked the Travis builds and they seem to be working with the changes to master. If you can kick appveyor, we should be able to merge.

[nicoddemus]

Hi everyone, sorry for barging in, but was this regression related to pytest's 3.1.0 warnings capture?

[nateprewitt]

Hey @nicoddemus, no need to apologize 😊 The regression is unrelated to the pytest 3.1.0 changes but those changes were effecting our build when this PR was opened. We've since implemented the suggested workaround in #4056.

[nicoddemus]

Great, thanks for the answer! 👍

[sigmavirus24]

So I'm not in favor of merging this. As was said earlier, this is not a public API.

[nateprewitt]

@sigmavirus24, if we're not going to maintain this as a public API, it should probably be moved to _internal_utils.py? The doctstring for utils.py states "This module provides utility functions that are used within Requests that are also useful for external consumption." which to me reads they're fair game for public use.

Edit: or modify the docstring. There's a fair amount of stuff in utils.py that probably shouldn't be depended on externally.

[codecov-io]

Codecov Report

Merging #4052 into master will increase coverage by 1.5%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #4052     +/-   ##
=========================================
+ Coverage   88.15%   89.65%   +1.5%     
=========================================
  Files          15       16      +1     
  Lines        1941     1962     +21     
=========================================
+ Hits         1711     1759     +48     
+ Misses        230      203     -27

Impacted Files	Coverage Δ
requests/utils.py	86% <100%> (ø)	⬆️
requests/__version__.py	100% <0%> (ø)
requests/adapters.py	92.92% <0%> (+0.47%)	⬆️
requests/models.py	93.42% <0%> (+2.26%)	⬆️
requests/_internal_utils.py	100% <0%> (+6.25%)	⬆️
...es/urllib3/packages/ssl_match_hostname/__init__.py	92.68% <0%> (+9.34%)	⬆️
requests/packages/chardet/compat.py	100% <0%> (+35.13%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ba21a7...a9470fe. Read the comment docs.