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

Add note about filtering warning #7839

Closed
wants to merge 2 commits into from
Closed

Conversation

Dreamsorcerer
Copy link
Member

No description provided.

@Dreamsorcerer Dreamsorcerer added bot:chronographer:skip This PR does not need to include a change note backport-3.9 labels Nov 14, 2023
Copy link

codecov bot commented Nov 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8ae650b) 97.40% compared to head (9e58f32) 97.40%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7839   +/-   ##
=======================================
  Coverage   97.40%   97.40%           
=======================================
  Files         106      106           
  Lines       32142    32142           
  Branches     3738     3738           
=======================================
  Hits        31307    31307           
  Misses        631      631           
  Partials      204      204           
Flag Coverage Δ
CI-GHA 97.31% <ø> (ø)
OS-Linux 96.99% <ø> (ø)
OS-Windows 95.48% <ø> (ø)
OS-macOS 96.66% <ø> (-0.01%) ⬇️
Py-3.10.11 95.41% <ø> (ø)
Py-3.10.13 96.85% <ø> (ø)
Py-3.11.6 96.50% <ø> (ø)
Py-3.12.0 96.57% <ø> (?)
Py-3.8.10 95.38% <ø> (ø)
Py-3.8.18 96.77% <ø> (ø)
Py-3.9.13 95.38% <ø> (ø)
Py-3.9.18 96.81% <ø> (+<0.01%) ⬆️
Py-pypy7.3.13 96.26% <ø> (ø)
VM-macos 96.66% <ø> (-0.01%) ⬇️
VM-ubuntu 96.99% <ø> (ø)
VM-windows 95.48% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

docs/web_advanced.rst Outdated Show resolved Hide resolved

If warnings are explicitly enabled (e.g. ``-We`` or ``-Wall``) then
using :class:`str` keys will trigger a warning. Like other warnings
this can be filtered out easily if you have no interest in this feature.

Choose a reason for hiding this comment

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

pet peeve! 'easy' should never be used in documentation; things are easy when you know how to do them, if you are reading documentation then this is prior to you knowing how to do them :)

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point! I occasionally hear tech writers mentioning this, but it's so often overlooked. We need to do better!

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really mind either way, but I personally don't get it. It's easy in the context of the documentation, as you only need to copy a line of code or CLI argument. Plenty of things are still difficult after you know how to do them. The word doesn't add anything of value to the docs though, so it makes sense to remove it for that reason, if we do merge it in.

Copy link
Member

Choose a reason for hiding this comment

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

@Dreamsorcerer it appears we failed to convey the complete context of the sentiment.
The point here is not that it's objectively easy or hard in the context, but how it affects the receiving side of the text (the reader).
When people hear (see) certain words, they have an effect on their perception. It's a subconscious thing. Hearing “just” often makes people feel dumb. They get defensive and don't accept the information as easily. This is not the recommendation for docs only, it's applied to all the teaching contexts (tutoring/mentoring/training/coaching/public speaking etc.)

The solution that is typically suggested is avoiding the use of such trigger words (or phrases). The same goes for saying “easy” / “straightforward” / “obvious” / “intuitive”. All of these have a great potential to discourage and hurt self-confidence of the readers. The less experience one has, the more harmful it is. This is a straight way to diminish people's feeling of achievement.

Instead, of using these, it's recommended to just skip to the explanation, without showing how much the writer is smarter than a reader.

Choose a reason for hiding this comment

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

@webknjaz I made a toot; i think we can stop annoying Dreamsorcerer any more :)
https://mastodon.green/@eoghanmurray/111521647225842098

If warnings are explicitly enabled (e.g. ``-We`` or ``-Wall``) then
using :class:`str` keys will trigger a warning. Like other warnings
this can be filtered out easily if you have no interest in this feature.
e.g. Add ``-W 'ignore:It is recommended to use web.AppKey instances for keys.'``
Copy link
Member

Choose a reason for hiding this comment

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

I usually prefer to be as narrow as possible when making such exclusions:

Suggested change
e.g. Add ``-W 'ignore:It is recommended to use web.AppKey instances for keys.'``
e.g. Add ``-W 'ignore:It is recommended to use web.AppKey instances for keys.:aiohttp.web_exceptions.NotAppKeyWarning:aiohttp.web_app'``

(although I'm not sure that the module part of the spec is right, because of the stacklevel, maybe drop it from my suggestion)

Can we advertise this? Besides, it's an “advanced” document.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm afraid it's not valid either way, otherwise I would have just used the type alone which is perfectly accurate: python/cpython#66733

I'm tempted to not merge this in, as nobody else has reported any concerns. The original report was from someone using 3.9.0b0, which wasn't filtering out the warnings by default. Nobody who explicitly enabled warnings has expressed any concerns with the presence of these warnings.

Copy link
Member

Choose a reason for hiding this comment

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

It's not? For some, I was convinced that I saw this work in cases when importing the exception wasn't causing it...

Copy link
Member Author

Choose a reason for hiding this comment

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

It basically only works when you manipulate the PYTHONPATH beforehand: python/cpython#66733 (comment)
Otherwise, the warnings filters are resolved before the import paths are resolved. Requiring PYTHONPATH manipulation is not a robust enough experience to recommend in the docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll close this for now, as I've not seen any other user reports. Will reopen it if anybody reports any concerns over the warning.

@Dreamsorcerer Dreamsorcerer deleted the Dreamsorcerer-patch-6 branch February 5, 2024 22:21
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.

3 participants