warn if handle_request returns dict

[AvdN]

This change puts a warning in the logger if a call to handle_request returns a dict instead of response object.

The warning references the file and line number of the "offending" handle_request method and a link
to the Response Objects documentation.

The code could be changed to output the warning only once, but that might make the warning go unnoticed.

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02% 🎉

Comparison is base (cf90d2d) 71.74% compared to head (d3e77bd) 71.77%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #449      +/-   ##
==========================================
+ Coverage   71.74%   71.77%   +0.02%     
==========================================
  Files          83       83              
  Lines        5713     5718       +5     
  Branches     1230     1231       +1     
==========================================
+ Hits         4099     4104       +5     
  Misses       1336     1336              
  Partials      278      278              

Files Changed	Coverage Δ
lona/view_runtime.py	82.07% <100.00%> (+0.25%)	⬆️

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

[fscherf]

@AvdN: Good idea! Should we use logging.warn or https://docs.python.org/3/library/warnings.html instead?
Also, could you please add the description of this PR to the patch and the prefix views:?

[AvdN]

If you use a DeprecationWarning or PendingDeprecationWarning there are IMO two problems. The first is that you don't actually see the warning anywhere by default. You have to either start Python with -X dev; set the env. variable PYTHONDEVMODE to 1; or, probably the most appropriate if you always want to warn people, after importing warnings in the program invoke warnings.simplefilter('always', PendingDeprecationWarning).

The second, bigger, problem is that you get the path + filename view_runtime.py and line number in that file. You can suppress those, but that is not entirely useful.

I chose logger.warn because I thought (but did not investigate) was what is used to get the messages after starting the server. BTW the warning get nicely colored (yellowish, orange).

I did notice right now that there is a problem with expanding the first warning string, which I had to change because the linter didn't like f-strings there. I have to look into that.

As for the description of the PR in the patch, you probably mean in the commit message (to make it more than one line), and have the first line read views:warn if handle_request returns dict (sorry I am not a git user).

[AvdN]

One thing that might be useful is keep a set of file-name, line-number tuples and only show the warning once for each combo. That set would have to be a ViewRuntime attribute, but could be created only if there is an actual handle_request that returns a dict.

[fscherf]

I see. But if possible I would want to use the official mechanism. Maybe we should instead configure warnings on Lona startup to show at least all Lona related warnings

[AvdN]

You can define LonaHandleRequestDictPendingDeprecationWarning as a subclass of PendingDeprecationWarning and then use simplefilter('always', ...) to show that. Then also monkey-patch warnings.filterwarnings to not display the standard file name and linenumber (the useless ones from view_runtime.py), and have the message include that info from the actual offending code. I have done that before.

I am not sure if that is more of an official mechanism than using logging.getLogger('....').warning(), but it has the advantage that users of lona can easily switch it off in their code (with another simplefilter on that class)

[fscherf]

@sobolevn: Do you have an opinion on that?

[AvdN]

I created a question on stackoverflow with an extracted/abstracted version of this problem. Please have a look at that (and the answer).

https://stackoverflow.com/q/76769281/1307905

[fscherf]

@AvdN: Nice! That would be a good solution I think

[AvdN]

Do you want me to make a new PR, or wait for comment from sobolevn? A new PR with the dependency on ruamel.std.warnings==0.2.2 (my preference), or the "standalone" solution (first part of the answer on SO)?

Any preference for the name of the PendingDeprecrationWarning subclass?

You can use PendingDeprecationWarning directly, but if you subclass it like

from lona import VERSION

if VERSION < (2, ):
    class ReturnDictDeprecationWarning(PendingDeprecationWarning):
        pass
else:
    class ReturnDictDeprecationWarning(DeprecationWarning):
        pass

warnings.filterwarning('once', ReturnDictDeprecationWarning)

you can more easily use it in other places as well, and get an "auto deprecation" when you update the version (of course you still need to update the code to actually no longer handle the dict return).

[fscherf]

@AvdN: Feel free to start a PR. I would prefer a standalone solution.
I would call the warning DictResponseDeprecationWarning