Skip to content
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 connectionpool.ConnectionPool.close() #874

Merged
merged 1 commit into from May 24, 2016

Conversation

tharvik
Copy link
Contributor

@tharvik tharvik commented May 24, 2016

See #873 for reference.

@Lukasa
Copy link
Sponsor Contributor

Lukasa commented May 24, 2016

Hrrrm, those test failures seem to be unrelated to this change: something is going on with install. I'll investigate.

@Lukasa
Copy link
Sponsor Contributor

Lukasa commented May 24, 2016

Cool, we bumped into this. No point piling on, the reports are already there, so let's just work around.

@Lukasa
Copy link
Sponsor Contributor

Lukasa commented May 24, 2016

Fab, so I've proposed a fix that should resolve the busted tests in #875. When that (or something like it) gets merged, you'll need to rebase on top of it to solve the test failures.

Sorry about that inconvenience!

@haikuginger
Copy link
Contributor

While we're in here, should we switch from pass to a NotImplemented exception akin to RequestMethods.urlopen()?

@tharvik
Copy link
Contributor Author

tharvik commented May 24, 2016

While we're in here, should we switch from pass to a NotImplemented exception akin to RequestMethods.urlopen()?

That would be even nicer, I'll do it when #875 is merged.

@Lukasa
Copy link
Sponsor Contributor

Lukasa commented May 24, 2016

@haikuginger @tharvik I don't actually think we want to do that. I think we shouldn't punish people who unconditionally call super() in close(), and it gives us the opportunity to be a bit more flexible later.

@haikuginger
Copy link
Contributor

I'll defer to @Lukasa's judgement on that, then; I'm 👍 on this change.

@tharvik
Copy link
Contributor Author

tharvik commented May 24, 2016

Okay, I won't add anything to the PR then.

@Lukasa
Copy link
Sponsor Contributor

Lukasa commented May 24, 2016

All better, mergy mergy mergy. Thanks @tharvik! ✨

@Lukasa Lukasa merged commit 126e8e9 into urllib3:master May 24, 2016
@sigmavirus24
Copy link
Contributor

@Lukasa you did the merge, don't forget the release notes ;)

Lukasa added a commit to Lukasa/urllib3 that referenced this pull request May 24, 2016
@haikuginger
Copy link
Contributor

Thanks very much @tharvik! 🍰

shazow added a commit that referenced this pull request May 24, 2016
Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull request Mar 16, 2022
fix connectionpool.ConnectionPool.close()
Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull request Mar 16, 2022
Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull request Mar 16, 2022
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.

None yet

4 participants