Opened 14 months ago

Last modified 3 weeks ago

#35667 assigned Cleanup/optimization

Switch usage to skip_file_prefixes instead of stacklevel when it makes sense

Reported by: Simon Charette Owned by: Luna
Component: Utilities Version: dev
Severity: Normal Keywords: deprecation stacklevel
Cc: Adam Johnson Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

In most of our usages of warnings.warn (and almost every usage that relates to deprecation warnings) we want them to be associated to the first out-of-as that's the most likely location that can be adjusted to avoid the warning.

In order to achieve this goal we've historically attempted to pass a fixed stacklevel to warnings.warn which can sometimes be tricky and error prone depending on how nested and convoluted the user or third-party app offending call site might be. In other cases we've opted not to provide a stacklevel at all as determining the offending call site under all circumstances is impossible.

Well it appears that this is a problem that Python 3.12 allows frameworks to address in a better way with the introduction of warnings.warn(skip_file_prefixes: tuple[str] | None). This new feature would allow us to ensure that the proper offending call site is referenced when emitting from a deeply nested call site where stacklevel is inappropriate.

Since this is a Python 3.12+ feature I'd suggest we introduce a get_non_django_stacklevel() -> int (better name welcome) function that could be used to pass to warnings.warn(stacklevel) until we drop support for Python 3.11.

Change History (18)

comment:1 by Simon Charette, 14 months ago

Summary: Switch warnings.warn usage to skip_file_prefixes instead of stacklevelSwitch usage to skip_file_prefixes instead of stacklevel when it makes sense

comment:2 by Natalia Bidart, 14 months ago

Keywords: deprecation stacklevel added
Triage Stage: UnreviewedAccepted
Version: 5.1dev

Hello Simon, as always thank you for your detailed and thorough report.

I agree with your proposal, I think this would help reducing the mistakes and overall improve the contributing experience. I'm setting this version to dev since this new feature should target 5.2 at this point.

comment:3 by Mohammad Salehi, 14 months ago

Hello, If you agree, I would like to review this issue and start working on it ?
Thank you.

comment:4 by Simon Charette, 14 months ago

I think we might want to wait for the patch for #35666 to land before committing efforts here Mohammad.

comment:5 by Jae Hyuck Sa , 13 months ago

Owner: set to Jae Hyuck Sa
Status: newassigned

comment:6 by Jae Hyuck Sa , 13 months ago

Has patch: set

comment:7 by Natalia Bidart, 11 months ago

Patch needs improvement: set

comment:8 by JaeHyuckSa, 11 months ago

Owner: changed from Jae Hyuck Sa to JaeHyuckSa

comment:9 by Jacob Walls, 6 weeks ago

Just noting we have a someday/maybe ticket to favor importlib.resources over __file__ to better support byte-compiled Django: #30950. Should we be using that when developing this feature?

comment:10 by Jacob Walls <jacobtylerwalls@…>, 6 weeks ago

In 34bd3ed:

Refs #36559, #35667 -- Used skip_file_prefixes in PartialTemplate.source warning.

comment:11 by GitHub <noreply@…>, 4 weeks ago

In 7b26b64:

Refs #35667 -- Cached Django file prefixes for warnings.

comment:12 by GitHub <noreply@…>, 4 weeks ago

In 8956ee3c:

Refs #35667 -- Updated contributing guide to use django_file_prefixes on deprecations.

comment:13 by GitHub <noreply@…>, 4 weeks ago

In 9932866e:

Refs #35667 -- Corrected usage of skip_file_prefixes in contributing docs.

comment:14 by JaeHyuckSa, 3 weeks ago

Owner: JaeHyuckSa removed
Status: assignednew

comment:15 by Simon Charette, 3 weeks ago

Now that we're on Python 3.12+ in main this issue is much easier as we don't have to build our own shim and can simply swap usages of stacklevel with skip_file_prefixes.

comment:16 by Jacob Walls <jacobtylerwalls@…>, 3 weeks ago

In fd705912:

Refs #36152, #35667 -- Used skip_file_prefixes in alias deprecation warning.

Follow-up to 8ede411a81b40ca53362e6788601193c7e56a0cf.

comment:17 by Luna, 3 weeks ago

Owner: set to Luna
Status: newassigned

comment:18 by Jacob Walls, 3 weeks ago

Note: See TracTickets for help on using tickets.
Back to Top