Emit UserWarning when accessing Application items by str #7553
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7553 +/- ##
=======================================
Coverage 97.35% 97.35%
=======================================
Files 106 106
Lines 31481 31490 +9
Branches 3575 3579 +4
=======================================
+ Hits 30648 30657 +9
Misses 630 630
Partials 203 203
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
I think this makes most sense in the context of #7555, so let's discuss that one first. In it's current form, you would only be able to access a string key after setting a string key, which means a warning would already have been emitted. I'd prefer not to spam the user constantly with warnings (note that we are not currently planning to deprecate string keys, I just figured that most users who enable warnings are probably the same users who want type safety). |
Not sure this really depends on #7555. When I first changed the keys to
That's valid, although if they don't want to change it, they might already ignore it and that would hide the new locations, too. It's easy enough to do. As mentioned earlier, I believe these new warning locations would help the users who want to update. |
In that case, you'd normally have got a KeyError, so the warning would be irrelevant. However, now I've understood the Home Assistant case, what do you think about adding these warnings, but having aiohttp set the ignore filter by default, then home assistant can override that and reenable the warnings to ensure that extension developers get alerted that they are using the deprecated keys (assuming that's possible)? |
|
My theory being that in aiohttp/web/init.py we can do: Then in home assistant, anytime after importing aiohttp.web, you can just do the same thing, changing it to "default" or "always". |
How is this going to interact with Maybe it would be better to add a custom warnings category for it to make it easier to ignore those? |
Well, the default test should test the default behaviour, i.e. no warnings (which should already be covered by us erroring on warnings). You can then test the warning behaviour using
I'm not sure, I figure we're just filtering out our own warning by default which is easily overriden. I've just noticed we can also use
Well, yes, we probably want to do something like that, if it's not trivial to filter on the current one alone (I don't know if |
|
I'll set the PR to I don't think this is really necessary for |
|
With the 3.9 testing, I think my proposal appears more difficult now. If we add the ignore (which is what I've needed to do for the existing warning), then it will still appear when running with I'm not sure there's any way to have a warning that is explicitly opt-in. So, if we want to add this, we would probably need to add some kind of configuration option in aiohttp itself (like |
|
What's the status of this in homeassistant? Do we need to revisit this? |
So far I had hold of on adding If you want, we can close this PR. In case there is still a need later, I can always reopen it or create a new one. |
What do these changes do?
In addition to emitting a
UserWarningwhen setting astrkey for Application items, also emit a warning when accessing it. Either viagetor__getitem__. This will help users to update their code more easily.I haven't added a CHANGES entry yet. Since the feature hasn't been shipped yet, it should still be covered with the original message. Please let me know if I should add one regardless.
aiohttp/CHANGES/5864.feature
Line 1 in cf97e5b
/CC @Dreamsorcerer
Are there changes in behavior for the user?
The
UserWarningwill be emitted in more cases. Not just hereaiohttp/aiohttp/web_app.py
Lines 169 to 178 in cf97e5b