Skip to content

Increase stacklevel for UserWarning about AppKey usage #7484

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

Merged
merged 2 commits into from
Aug 6, 2023

Conversation

cdce8p
Copy link
Contributor

@cdce8p cdce8p commented Aug 6, 2023

What do these changes do?

The UserWarning to use web.AppKey should be emitted on the call site.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@Dreamsorcerer
Copy link
Member

Thanks for trying this out. Do you think it's possible to tweak the test to catch this? https://github.com/aio-libs/aiohttp/blob/master/tests/test_web_app.py#L147

@codecov
Copy link

codecov bot commented Aug 6, 2023

Codecov Report

Merging #7484 (f4f185d) into master (1fb06bb) will decrease coverage by 0.01%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7484      +/-   ##
==========================================
- Coverage   97.27%   97.27%   -0.01%     
==========================================
  Files         106      106              
  Lines       31448    31442       -6     
  Branches     3567     3568       +1     
==========================================
- Hits        30592    30586       -6     
  Misses        652      652              
  Partials      204      204              
Flag Coverage Δ
CI-GHA 97.22% <100.00%> (-0.01%) ⬇️
OS-Linux 96.89% <100.00%> (-0.01%) ⬇️
OS-Windows 95.34% <100.00%> (-0.01%) ⬇️
OS-macOS 96.57% <100.00%> (-0.01%) ⬇️
Py-3.10.11 95.27% <100.00%> (-0.01%) ⬇️
Py-3.10.12 96.78% <100.00%> (-0.01%) ⬇️
Py-3.11.4 96.48% <100.00%> (-0.02%) ⬇️
Py-3.8.10 95.24% <100.00%> (-0.01%) ⬇️
Py-3.8.17 96.71% <100.00%> (-0.01%) ⬇️
Py-3.9.13 95.23% <100.00%> (-0.01%) ⬇️
Py-3.9.17 96.74% <100.00%> (-0.01%) ⬇️
Py-pypy7.3.11 96.29% <100.00%> (-0.01%) ⬇️
VM-macos 96.57% <100.00%> (-0.01%) ⬇️
VM-ubuntu 96.89% <100.00%> (-0.01%) ⬇️
VM-windows 95.34% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
aiohttp/web_app.py 95.25% <ø> (ø)
tests/test_web_app.py 99.47% <100.00%> (+<0.01%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Dreamsorcerer Dreamsorcerer added bot:chronographer:skip This PR does not need to include a change note backport-3.9 labels Aug 6, 2023
@cdce8p
Copy link
Contributor Author

cdce8p commented Aug 6, 2023

Thanks for trying this out. Do you think it's possible to tweak the test to catch this? https://github.com/aio-libs/aiohttp/blob/master/tests/test_web_app.py#L147

Pytest doesn't really support it unfortunately pytest-dev/pytest#4343. I've tried a small workaround to at least check the file name where the warning is emitted but this is not something I would recommend for everything. Let me know what you think.

@Dreamsorcerer
Copy link
Member

That's probably fine.

@Dreamsorcerer Dreamsorcerer enabled auto-merge (squash) August 6, 2023 16:41
@Dreamsorcerer Dreamsorcerer merged commit 466f1eb into aio-libs:master Aug 6, 2023
@patchback
Copy link
Contributor

patchback bot commented Aug 6, 2023

Backport to 3.9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.9/466f1ebbac1bd4047d0eeaed582ac12834bad305/pr-7484

Backported as #7489

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Aug 6, 2023
@cdce8p cdce8p deleted the app-stacklevel branch August 6, 2023 17:04
Dreamsorcerer pushed a commit that referenced this pull request Aug 6, 2023
… about AppKey usage (#7489)

**This is a backport of PR #7484 as merged into master
(466f1eb).**

## What do these changes do?

The `UserWarning` to use `web.AppKey` should be emitted on the call
site.

## Checklist

- [x] I think the code is well written
- [ ] Unit tests for the changes exist
- [ ] Documentation reflects the changes
- [x] If you provide code modification, please add yourself to
`CONTRIBUTORS.txt`
  * The format is &lt;Name&gt; &lt;Surname&gt;.
  * Please keep alphabetical order, the file is sorted by names.
- [ ] Add a new news fragment into the `CHANGES` folder
  * name it `<issue_id>.<type>` for example (588.bugfix)
* if you don't have an `issue_id` change it to the pr id after creating
the pr
  * ensure type is one of the following:
    * `.feature`: Signifying a new feature.
    * `.bugfix`: Signifying a bug fix.
    * `.doc`: Signifying a documentation improvement.
    * `.removal`: Signifying a deprecation or removal of public API.
* `.misc`: A ticket has been closed, but it is not of interest to users.
* Make sure to use full sentences with correct case and punctuation, for
example: "Fix issue with non-ascii contents in doctest text files."

Co-authored-by: Marc Mueller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:skip This PR does not need to include a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants