Skip to content

Fix deprecation warning text and location - #3859

Open
takayoshi-makabe wants to merge 3 commits into
apache:mainfrom
takayoshi-makabe:fix-deprecation-warnings
Open

Fix deprecation warning text and location#3859
takayoshi-makabe wants to merge 3 commits into
apache:mainfrom
takayoshi-makabe:fix-deprecation-warnings

Conversation

@takayoshi-makabe

Copy link
Copy Markdown

Rationale for this change

Deprecation warnings are wrong in two ways: what they say, and where they point.

The text leaks a literal None. help_message is optional in all three public helpers, but it is interpolated straight into the message, so omitting it renders str(None):

@deprecated(deprecated_in="0.12.0", removed_in="0.13.0")
def foo() -> None: ...
DeprecationWarning: Call to foo, deprecated in 0.12.0, will be removed in 0.13.0.None

deprecation_notice(..., help_message=None) has the same problem.

The warning is attributed to PyIceberg, not to the caller. warnings.warn used to be called inline from the decorator's wrapper, where stacklevel=2 pointed at the caller. #962 extracted it into the shared _deprecation_warning helper and kept stacklevel=2, which added a frame:

pyiceberg/utils/deprecated.py:35: DeprecationWarning: Call to foo, ...

That hides the one thing a reader needs — which of their own calls to change. Both callers, the decorator's wrapper and deprecation_message, sit exactly one frame above the helper, so stacklevel=3 fixes both.

An empty help_message is treated the same as no message. It is not a meaningful help string, and is not None would render a dangling " .".

Are these changes tested?

Yes. tests/utils/test_deprecated.py gains cases for all three helpers without a help message, with deprecation_notice parametrized over None and "". The existing tests assert the same strings as before.

The attribution is covered separately: the existing tests patch warnings.warn, so they can only see the arguments, not where the warning lands. Two new tests record real warnings and assert the reported filename is the caller's — they fail on stacklevel=2.

Are there any user-facing changes?

The warning text no longer ends in None when a call site omits the help message, and warnings now report the caller's file and line. Both in-repo call sites pass a help message, so no emitted text changes today; the attribution fix applies to every warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant