Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix deprecation warning stacklevel #1610

Merged
merged 1 commit into from
Sep 30, 2018
Merged

Fix deprecation warning stacklevel #1610

merged 1 commit into from
Sep 30, 2018

Conversation

Zac-HD
Copy link
Member

@Zac-HD Zac-HD commented Sep 30, 2018

stacklevel=3 used to point at the call site in user code, but now points into the lazy reification machinery. Changing that to stacklevel=2 points it back at the call to note_deprecation, which is still useful.

Closes #652.

@Zac-HD Zac-HD added the legibility make errors helpful and Hypothesis grokable label Sep 30, 2018
Zalathar
Zalathar reviewed Sep 30, 2018
View reviewed changes
hypothesis-python/tests/nocover/test_regressions.py Outdated
record, = log
# We got the warning we expected, from the right file
assert isinstance(record.message, HypothesisDeprecationWarning)
assert record.message.args == ('message',)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nitpick: Maybe change 'message' to something more distinctive, just to make it abundantly clear to the reader that this is a placeholder message that should match the one used in the strategy above.

Zalathar
Zalathar previously approved these changes Sep 30, 2018
View reviewed changes
Copy link
Contributor

@Zalathar Zalathar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@Zalathar Zalathar dismissed their stale review September 30, 2018 01:13

Never mind, I want to personally understand the problem more before I approve this.

@Zac-HD
Fix deprecation warning stacklevel
8343c64 
stacklevel=3 used to point at the call site in user code, but now points into the lazy reification machinery.
@Zac-HD
Copy link
Member Author

Zac-HD commented Sep 30, 2018

No worries! Changing the filter to 'error' in the new test will give you the full traceback, so you can see the frames in question, and from there I found that reading warnings.warn made it pretty obvious what was going on.

@Zalathar
Copy link
Contributor

Zalathar commented Sep 30, 2018 •
edited
Loading

So, as I understand it:

  • Originally the point of stacklevel=3 (way back in 4a371de) was to make the stack trace start in user code.
  • Over time, as more deprecations were made, this wasn't actually doing the right thing in all cases.
  • Because note_deprecation is called in lots of different places and contexts, it's currently not practical to make it consistently point to user code.
  • Given that, it should at least consistently point to the internal code that called note_deprecation, because that's better than pointing to an arbitrary internal stack frame one level up.

Strictly speaking this doesn't “fix” #652, but at least it's now not-fixed in a sensible way, instead of a completely baffling way, which is an improvement.

@Zac-HD
Copy link
Member Author

Zac-HD commented Sep 30, 2018

Yep, that's all correct - I'm not sure there are any cases left where stacklevel=3 is pointing at user code, but that's just a matter of degree.

@Zalathar
Copy link
Contributor

In that case, I'm satisfied that this patch makes sense and is an improvement.

(It would be nice to restore user-code stack traces in the cases where that's possible, but such work would be well beyond the scope of this PR.)

@Zac-HD Zac-HD merged commit 174f595 into HypothesisWorks:master Sep 30, 2018
@Zac-HD Zac-HD deleted the warning-stacklevel branch September 30, 2018 14:18
@Zac-HD
Copy link
Member Author

Zac-HD commented Sep 30, 2018

🎉

For what it's worth, #1582 will point back to user code for anyone running with warnings as errors!

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

@Zalathar Zalathar Zalathar approved these changes

Assignees
No one assigned
Labels
legibility make errors helpful and Hypothesis grokable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Zac-HD @Zalathar