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 several ResourceWarnings in test_requests.py #4766

Merged
merged 1 commit into from May 11, 2022
Merged

Fix several ResourceWarnings in test_requests.py #4766

merged 1 commit into from May 11, 2022

Conversation

BoboTiG
Copy link
Contributor

@BoboTiG BoboTiG commented Aug 13, 2018

Hello,

I do not know if you are interested in fixing ResourceWarnings in tests, but I will take my chance :)
There was 10 unclosed files in test_requests.py.

@codecov-io
Copy link

codecov-io commented Aug 13, 2018

Codecov Report

Merging #4766 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4766   +/-   ##
=======================================
  Coverage   66.89%   66.89%           
=======================================
  Files          15       15           
  Lines        1577     1577           
=======================================
  Hits         1055     1055           
  Misses        522      522

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 5c1f72e...fdab4fb. Read the comment docs.

panmpan17
panmpan17 previously approved these changes Sep 13, 2018
Copy link
Member

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @BoboTiG, thanks for looking into this. I don't have an issue adding the context handlers, but we should probably avoid dumping all of the logic inside of them.

If we want to encourage best practices, let's make sure anything not relevant to the file handler stays at its original indentation. Once that's done, if you can rebase onto master, I think I'm happy with this.

tests/test_requests.py Outdated Show resolved Hide resolved
tests/test_requests.py Outdated Show resolved Hide resolved
tests/test_requests.py Outdated Show resolved Hide resolved
tests/test_requests.py Outdated Show resolved Hide resolved
tests/test_requests.py Outdated Show resolved Hide resolved
@BoboTiG
Copy link
Contributor Author

BoboTiG commented Oct 1, 2018

@nateprewitt, thanks for the review. You are totally right and I applied changes + rebase on master :)

We only need to keep r.prepare*() at the same indentation level as the definition of r to prevent models.py:159: ValueError: read of closed file.

@BoboTiG
Copy link
Contributor Author

BoboTiG commented Jan 4, 2019

What is the status of the PR? Would you mind trigger the request-CI job?

@BoboTiG
Copy link
Contributor Author

BoboTiG commented Oct 31, 2019

Ping :)

@nateprewitt nateprewitt changed the base branch from master to main January 3, 2022 15:30
Copy link
Member

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @BoboTiG, I've rebased your change onto the top of our main branch and resolved some conflicts. This seems like a reasonable fix still and I think we're happy with its current state. Thanks again for the contribution!

@nateprewitt nateprewitt merged commit cb233a1 into psf:main May 11, 2022
@BoboTiG BoboTiG deleted the fix-resource-warnings branch May 11, 2022 05:57
joelkak pushed a commit to joelkak/requests that referenced this pull request May 12, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants