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.

Sign up for GitHub

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

Add note about filtering warning #7839

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f4851e5
Add note about filtering warning
Dreamsorcerer Nov 14, 2023
9e58f32
Update docs/web_advanced.rst
Dreamsorcerer Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion docs/web_advanced.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,8 @@ and get it back in the :term:`web-handler`::
data = request.app['my_private_key']

Rather than using :class:`str` keys, we recommend using :class:`AppKey`.
This is required for type safety (e.g. when checking with mypy)::
This is required for type safety (e.g. when checking with mypy,
or for auto-complete in an IDE)::

my_private_key = web.AppKey("my_private_key", str)
app[my_private_key] = data
Expand All @@ -403,6 +404,16 @@ This is required for type safety (e.g. when checking with mypy)::
data = request.app[my_private_key]
# reveal_type(data) -> str

.. note::

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

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.

Copy link

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

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.

after ``-Wall`` or add
``warnings.filterwarnings("ignore", category=web.NotAppKeyWarning)`` to your
code.

In case of :ref:`nested applications
<aiohttp-web-nested-applications>` the desired lookup strategy could
be the following:
Expand Down
Loading