Skip to content

Fix several ResourceWarnings in test_requests.py#4766

Merged
nateprewitt merged 1 commit intopsf:mainfrom
BoboTiG:fix-resource-warnings
May 11, 2022
Merged

Fix several ResourceWarnings in test_requests.py#4766
nateprewitt merged 1 commit intopsf:mainfrom
BoboTiG:fix-resource-warnings

Conversation

@BoboTiG
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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.

Comment thread tests/test_requests.py Outdated
Comment thread tests/test_requests.py Outdated
Comment thread tests/test_requests.py Outdated
Comment thread tests/test_requests.py Outdated
Comment thread tests/test_requests.py Outdated
@BoboTiG
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

4 participants