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

Ensure rewritten modules don't inherit __future__ flags from pytest #2350

Merged
merged 1 commit into from
Apr 11, 2017

Conversation

nicoddemus
Copy link
Member

In a recent refactoring (#2316) I enabled all __future__ features in all pytest modules, but that has the unwanted side effect of propagating those features to compile()'d modules inside assertion rewriting, unless we pass dont_inherit=False to compile():

The optional arguments flags and dont_inherit control which future statements (see PEP 236) affect the compilation of source. If neither is present (or both are zero) the code is compiled with those future statements that are in effect in the code that is calling compile. If the flags argument is given and dont_inherit is not (or is zero) then the future statements specified by the flags argument are used in addition to those that would be used anyway. If dont_inherit is a non-zero integer then the flags argument is it – the future statements in effect around the call to compile are ignored.

Reference.

Wow it was lucky of us to catch this before the 3.1 release, I can't imagine how many suites would break by this! 😱

In a recent refactoring we enabled all __future__ features in pytest
modules, but that has the unwanted side effect of propagating those
features to compile()'d modules inside assertion rewriting, unless
we pass dont_inherit=False to compile().
@The-Compiler
Copy link
Member

Good catch! Looks like there are some other places calling compile() too though?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.577% when pulling 1b5f898 on nicoddemus:future-imports-rewrite into 83b241b on pytest-dev:features.

@nicoddemus
Copy link
Member Author

Hmm yeah, I meant to do that after checking this fixes our suites at work and forgot to come back to review the other compile calls.

Will do it later, thanks for the reminder.

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

im fine with merging this one as is already since it improves the situation, should other compile calls need extension feel free to merge after you finished the review

thanks for the good work :)

@nicoddemus
Copy link
Member Author

I did review the other usages of compile, and I think they don't need the inherit flag because they deal with only simple expressions instead of entire modules.

@nicoddemus nicoddemus merged commit 78ac1bf into pytest-dev:features Apr 11, 2017
@nicoddemus nicoddemus deleted the future-imports-rewrite branch April 11, 2017 23:59
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.

4 participants