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.

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

Adjust stacklevel of implprefix warning #181

Merged

Conversation

crazymerlyn
Copy link
Member

Fixes #166.

@crazymerlyn crazymerlyn force-pushed the change_implprefix_warning_stacklevel branch 2 times, most recently from 6aed39b to 98c3974 Compare October 16, 2018 05:58
@crazymerlyn crazymerlyn force-pushed the change_implprefix_warning_stacklevel branch from 98c3974 to bee289c Compare October 16, 2018 06:01
@crazymerlyn
Copy link
Member Author

The build is failing due to a recent change in pytest which adds a wrapper for warnings.warn function for python 2 (thus, changing the stack). Not sure what the best solution is here.
Adding a simple if 'stacklevel' in kwargs: kwargs['stacklevel'] += 1 in the wrapper function could work.

@goodboy
Copy link
Contributor

goodboy commented Oct 16, 2018

@crazymerlyn thanks for the PR 👍!
@nicoddemus might know the right tweak to make.

@nicoddemus
Copy link
Member

Adding a simple if 'stacklevel' in kwargs: kwargs['stacklevel'] += 1 in the wrapper function could work.

Oh indeed, good catch. What do you think @asottile?

@asottile
Copy link
Member

Adding a simple if 'stacklevel' in kwargs: kwargs['stacklevel'] += 1 in the wrapper function could work.

Oh indeed, good catch. What do you think @asottile?

oh! I see what you're saying now -- I was wracking my brain to figure out what was meant here.

if isinstance(kwargs.get('stacklevel'), int):
    kwargs['stacklevel'] += 1

might be safer

probably PR that to pytest and we can get it into 3.9.2?

@crazymerlyn
Copy link
Member Author

if isinstance(kwargs.get('stacklevel'), int):
    kwargs['stacklevel'] += 1

might be safer

probably PR that to pytest and we can get it into 3.9.2?

Yeah, that seems better. But while we are on this, I think stacklevel also needs to be changed if it is not present in kwargs. The default value is 1. So, if somebody uses recwarn in python2.7, the warning will present itself at the wrapper function rather than the line causing the warning.
How about this:

if isinstance(kwargs.get('stacklevel'), int):
    kwargs['stacklevel'] += 1
elif 'stacklevel' not in kwargs:
    kwargs['stacklevel'] = 2

@asottile
Copy link
Member

ah in that case even easier:

kwargs.setdefault('stacklevel', 1)
kwargs['stacklevel'] += 1

@crazymerlyn
Copy link
Member Author

So, I tried adding this change to a PR but got a few errors with test_deprecated_call_modes and test_can_capture_previously_captured. Not really sure what's going wrong (python apparently uses a c extension for warnings.warn so pdb wasn't much help).

@asottile
Copy link
Member

hmm yeah this is probably the __warningregistry__ bits. link your branch? I'll poke at it since I broke this 😭

@nicoddemus
Copy link
Member

I'll poke at it since I broke this

Heh, that's all good and part of the game 😁

@crazymerlyn
Copy link
Member Author

@asottile
Copy link
Member

not pretty, but at least this is throwaway :'( pytest-dev/pytest#4192

@crazymerlyn crazymerlyn reopened this Oct 25, 2018
@asottile
Copy link
Member

sorry about the trouble again, thanks for the report! 🎉

@goodboy
Copy link
Contributor

goodboy commented Nov 9, 2018

@nicoddemus good to go here yah?

@asottile
Copy link
Member

asottile commented Nov 9, 2018

yah!

@goodboy goodboy merged commit b40bcd8 into pytest-dev:master Nov 9, 2018
@crazymerlyn crazymerlyn deleted the change_implprefix_warning_stacklevel branch November 9, 2018 07:25
@edmorley
Copy link

edmorley commented Nov 9, 2018

Many thanks!

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.

5 participants