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

Importing hypothesis mutates global warnings state #618

Closed
Julian opened this issue May 11, 2017 · 8 comments
Closed

Importing hypothesis mutates global warnings state #618

Julian opened this issue May 11, 2017 · 8 comments
Assignees

Comments

@Julian
Copy link

Julian commented May 11, 2017

hypothesis.errors mutates the global warnings state:

https://github.com/HypothesisWorks/hypothesis-python/blob/master/src/hypothesis/errors.py#L182

This causes hypothesis to override any warnings settings that have already been applied. E.g., setting PYTHONWARNINGS="error" will not be respected, because hypothesis will change HypothesisDeprecationWarnings to be instead printed.

The filter there should presumably not do anything if the user has already modified any warnings defaults.

@DRMacIver
Copy link
Member

The filter there should presumably not do anything if the user has already modified any warnings defaults.

Agreed. Is there actually a way to do that though?

@Julian
Copy link
Author

Julian commented May 11, 2017

Yeah :/ I dunno if there's a built in way that isn't just reimplementing the cascading, I'd have to check -- "PYTHONWARNINGS" in os.environ covers obviously the environment case, but ideally the warnings module should have something that tells you whether there's something new happening. Will have a look after popping some more things off the yak stack.

@DRMacIver
Copy link
Member

It's easy, if undocumented, to get the list of filters applied - it's just warnings.filters. But I'm not sure that helps.

More generally, what counts as "modified"? e.g. other modules may want to add warnings rules about their own types. That shouldn't disable Hypothesis. We can't just check if there's a rule for an ancestor of HypothesisDeprecationWarning, because the default behaviour is to ignore deprecation warnings!

RE the environment suggestion: That then misses filters specified with '-W'.

Basically I'm very up for doing something better here, but it's not all that clear how.

@Julian
Copy link
Author

Julian commented May 11, 2017

Right agreed (and yeah I know about -W, I was just suggesting a first step towards at least fixing the problem in front of me :P) -- do you agree that the idea is "Hypothesis should not be doing anything that messes with someone who actually knows how to configure the warnings system"?

If it wants to try to help users who don't know about Python's bad defaults (by changing the default to show the warning), that might be reasonable -- personally though if it turns out that there's no way to do that without actually breaking people who are actually configuring it knowingly, I'd selfishly say that hypothesis is overstepping its boundaries there, but I have to read a bit more about what's possible, sounds like you remember more than I do at the moment :P.

Just thinking out loud, maybe one thing to do is to not actually have HypothesisDeprecationWarnings be DeprecationWarnings?

Will try to propose something concrete-r.

@DRMacIver
Copy link
Member

do you agree that the idea is "Hypothesis should not be doing anything that messes with someone who actually knows how to configure the warnings system"?

Yes

If it wants to try to help users who don't know about Python's bad defaults (by changing the default to show the warning), that might be reasonable

Yes, that's the goal here.

personally though if it turns out that there's no way to do that without actually breaking people who are actually configuring it knowingly, I'd selfishly say that hypothesis is overstepping its boundaries there

Maybe, but I'd be very resistant to changing it if you can't propose an alternative way to defend us from the hordes of people going "zomg this API went away why didn't you give us any warning?" when they've been ignoring the deprecation warning that Python hid by default for the last year. :-)

Just thinking out loud, maybe one thing to do is to not actually have HypothesisDeprecationWarnings be DeprecationWarnings?

I'm not against the idea, but I would consider that a breaking change (and don't know how to emit appropriate deprecation warnings for it! ;-) )

As a side note, recreating the warnings logic well enough to determine the warning level for HypothesisDeprecationWarning doesn't look too bad.

@Elliott-Boyer
Copy link

It appears that the issue is caused by the warnings.simplefilter in errors.py, I may be wrong but the way it appears to work is the command line arguments are stored in sys.warnoptions, when warnings is imported warnoptions are then inserted in the front of the filters, later on when the filter is defined in errors.py without the append = True flag being passed it then goes to the front, matches when the warning is thrown and the command line arguments are thus ignored.

A fix would therefore be to add the append = True flag to the filter

@Zac-HD
Copy link
Member

Zac-HD commented Aug 7, 2017

A fix would therefore be to add the append = True flag to the filter

Doing this, and verifying that it works (ie command-line arguments take precedence) would make a good first contribution to Hypothesis.

@Zac-HD Zac-HD self-assigned this Dec 14, 2017
@Zac-HD
Copy link
Member

Zac-HD commented Dec 14, 2017

PEP 565 has been accepted. While the main point is to tweak the default filter for DeprecationWarnings, it also has some recommendations for libraries - for us, the relevant one is that warnings intended for users should inherit from FutureWarning rather than DeprecationWarning.

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

No branches or pull requests

4 participants