django-commons / django-debug-toolbar

A configurable set of panels that display various debug information about the current request/response.
https://django-debug-toolbar.readthedocs.io
BSD 3-Clause "New" or "Revised" License
8.08k stars 1.05k forks source link

Deprecate and remove `OBSERVE_REQUEST_CALLBACK` #1886

Closed tim-schilling closed 4 months ago

tim-schilling commented 8 months ago

With the addition of UPDATE_ON_FETCH from #1843, we no longer need to keep OBSERVE_REQUEST_CALLBACK around. The functionality that can be implemented with that can be redone in SHOW_TOOLBAR_CALLBACK as indicated by @living180 here

kevalrajpalknight commented 4 months ago

@tim-schilling this issue could be marked as closed.

dennisvang commented 2 months ago

(debug_toolbar.W008) The deprecated OBSERVE_REQUEST_CALLBACK setting is present in DEBUG_TOOLBAR_CONFIG. HINT: Use the UPDATE_ON_FETCH and/or SHOW_TOOLBAR_CALLBACK settings instead.

This deprecation warning appears to suggest that we can remove OBSERVE_REQUEST_CALLBACK from the DEBUG_TOOLBAR_CONFIG dict.

However, that's not the case, because it would break the following:

https://github.com/jazzband/django-debug-toolbar/blob/9e30a06e418ecdd4eeb837530b86be40bb1e3d2d/debug_toolbar/toolbar.py#L182

Moreover, it is not clear to me from the message/docs/PR whether the default values for UPDATE_ON_FETCH and/or SHOW_TOOLBAR_CALLBACK are equivalent to the default value of OBSERVE_REQUEST_CALLBACK.

tim-schilling commented 2 months ago

@dennisvang OBSERVE_REQUEST_CALLBACK defaults to returning [a reference to the function] toolbar.observe_request which returns True. That's why it's safe to remove in a future update of the toolbar.

If you're attempting to have the toolbar reflect the latest request being made, including the AJAX requests while you're on a page, set UPDATE_ON_FETCH to True. If that's not helpful, please provide more information on what you're confused about and what you're trying to do.

dennisvang commented 2 months ago

@tim-schilling Thanks for the explanation.

My point is that we now get a warning if OBSERVE_REQUEST_CALLBACK is in the config dict, but removing it from the config dict causes an error, so it seems like there is no way to act on this warning yet.

tim-schilling commented 2 months ago

@dennisvang It should only log a deprecation warning if you include it in your project's settings' DEBUG_TOOLBAR_CONFIG dict (see https://github.com/jazzband/django-debug-toolbar/blob/9e30a06e418ecdd4eeb837530b86be40bb1e3d2d/debug_toolbar/apps.py#L265). Not including it in that dictionary should be fine (no errors and no warnings).

If you're getting a KeyError, I suspect you're manipulating the toolbar's actual default dict, which you probably shouldn't do unless you have good reason to.

dennisvang commented 2 months ago

@tim-schilling Thanks, you're right:

It turns out we had DEBUG_TOOLBAR_CONFIG = debug_toolbar.settings.CONFIG_DEFAULTS, without making a copy. So, removing the key from there would indeed manipulate the default dict.

Sorry for that.