Fix deprecation warning text and location - #3859
Open
takayoshi-makabe wants to merge 3 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_messageis optional in all three public helpers, but it is interpolated straight into the message, so omitting it rendersstr(None):deprecation_notice(..., help_message=None)has the same problem.The warning is attributed to PyIceberg, not to the caller.
warnings.warnused to be called inline from the decorator's wrapper, wherestacklevel=2pointed at the caller. #962 extracted it into the shared_deprecation_warninghelper and keptstacklevel=2, which added a frame: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, sostacklevel=3fixes both.An empty
help_messageis treated the same as no message. It is not a meaningful help string, andis not Nonewould render a dangling" .".Are these changes tested?
Yes.
tests/utils/test_deprecated.pygains cases for all three helpers without a help message, withdeprecation_noticeparametrized overNoneand"". 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 onstacklevel=2.Are there any user-facing changes?
The warning text no longer ends in
Nonewhen 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.