Allow __future__ in doctest

[jayvdb]

Deprecate Checker.futuresAllowed as it is now unused,
and is typically unnecessary.

[jayvdb]

fwiw, since contributors now need to be explicit to avoid maintainers fiddling with commits, I am capable of rebasing my own changesets if it is required, so please dont mess up my workflows by doing it for me.

[jayvdb]

bah, this wont work with stack levels

[bitglue]

Should this class (and while we're at it, maybe the one above) just be removed? Unless I'm missing something, not much point to an empty test class.

[jayvdb]

No. The mixin means this empty class runs all of the TestImports tests with a different self.flakes method.

[bitglue]

Ah, of course. Perhaps instead of pass we can have a short docstring saying just that?

[jayvdb]

Sure; I'll update this patch after #42 is merged, copying the docstring that is merged for that test case.

Anything else?

[bitglue]

What's the essential change being made here? I thought maybe __future__ imports weren't allowed in docstrings for some reason, but on master right now, this passes:

def f():
    """
        >>> from __future__ import print_function
        >>> 'a'
    """

[jayvdb]

Did you enable doctests when you tested that?

[bitglue]

Apparently not. Definitely need more coffee this morning :)

What do you think about dropping futuresAllowed entirely, rather than deprecating it? I doubt anything is using it unless flake8 is somehow (@sigmavirus24), and dropping it entirely would simplify things a lot.

[jayvdb]

I dont see how dropping it simplifies the code significantly. It is only 14 lines of deprecation code for the property.
_module_scopes (or similar) is needed in other doctest fixes (e.g. something similar could have been used by the GLOBAL fix #38).
IMO a new test module for Checker (test_checker.py and harness.py modifications) is necessary anyhow (I have three pending changesets to push which all introduce it, for one reason or another).

The underlying assumption about futuresAllowed is still true most of the time, and it is a reasonable proxy for 'does the module have real code in it yet'. It is only doctest where that assumption breaks down.

As a result, I could easily be convinced that futuresAllowed should only refer to the module's _futures_allowed, and not return True when in the doctests. That would simplify the deprecation support code a little.

futuresAllowed is part of the public interface, so it is part of the contract with users of the module, and it is the public interface of the core component of the package.

Not deprecating it implies this fix could only be released in pyflakes 2.0, if this project is using semver.

[bitglue]

Not deprecating it implies this fix could only be released in pyflakes 2.0, if this project is using semver.

It's not using semver, so I wouldn't worry about it too much. We could probably do better at clearly delineating the public interface. It's probably just:

• the pyflakes command-line executable
• the pyflakes.api module

It would be nice to preserve the Checker interface, but honestly, it's pretty horrible. So breaking compatibility for the sake of a better interface seems like a net win to me.

Perhaps it would help me to understand your objective if you could elaborate on what behaviors the tests are asserting. If the problem is that putting a __future__ import in a docstring yields a false positive, can't we just feed an example of such to self.flakes() and assert there are no errors?

[jayvdb]

As explained in the commit message, Checker now does not use futuresAllowed, so there is logically no way that using TestCase.flakes can assert that futuresAllowed backwards compatibility is retained. That is proper deprecation; remove usage within your own codebase and add test cases to ensure support for downstream users until it is decommissioned.

There are projects using Checker. It is part of the public interface, per PEP8.

[sigmavirus24]

It is part of the public interface, per PEP8.

What?

I took a quick look at flake8 and it's not using this attribute. What other projects use pyflakes via pyflakes.api @jayvdb (since you seem to have knowledge of several). The only other one I can think of (propsector) doesn't use Checker and I'm certain @carlio can confirm that my grep-foo hasn't failed me.

[jayvdb]

"per PEP8" = as defined by PEP8.

It should use underscore prefixes for private variables.

What other projects use pyflakes via pyflakes.api

I assume you mean "What other projects use pyflakes via Checker" ...?

https://github.com/rdunklau/Gedit-checkpython has used Checker since December 2011.

[sigmavirus24]

While that project uses Checker it doesn't use futuresAllowed: https://github.com/rdunklau/Gedit-checkpython/blob/master/checkpython/checkers.py#L120 (that's the only reference I see to its use of Checker). Any other projects?

[sigmavirus24]

Pylama seems to be maintained and using the Checker class without using futuresAllowed: https://github.com/klen/pylama/blob/develop/pylama/lint/pylama_pyflakes.py#L42

[sigmavirus24]

hghooks uses the class but doesn't care about futuresAllowed https://bitbucket.org/lgs/hghooks/src/313c7745eaf2327123674140fb5edc2f2842498f/hghooks/code.py?at=default&fileviewer=file-view-default#code.py-174

(My point here is that I'd bet 99% of people using the class use it only to access the .messages attribute).

[jayvdb]

@sigmavirus24 , that line of reasoning is beating around the bush.

It is not possible to know all uses; at best we could look at all usage in open source projects published and findable via Google. That is why there are conventions for public interfaces.

Is your input in this code review that you want me to break backwards compatibility with regards to Checker.futuresAllowed?

[ryneeverett]

Documented interfaces are considered public, unless the documentation explicitly declares them to be provisional or internal interfaces exempt from the usual backwards compatibility guarantees. All undocumented interfaces should be assumed to be internal.

[sigmavirus24]

@sigmavirus24 that line of reasoning is beating around the bush.

It isn't beating around the bush. It's pragmatic instead of ideological.

Is your input in this code review that you want me to break backwards compatibility with regards to Checker.futuresAllowed?

My input is that @bitglue's input makes sense and is reasonable (as they are the primary maintainer). Short of writing a new class to take the Checker's class, we can only improve the interface. Making futuresAllowed into a descriptor with a warning only benefits people actually testing their modules given the fact that warnings are off by default in newer versions of Python.

Finally, there is no documentation around futuresAllowed. Your argument is invalid. If what we can see indicates that people are primarily using the Checker class in one specific way, why are we arguing over this? Is it because you don't want to break it? Fine. I'll send the PR to remove the code. It's the same result.

[bitglue]

Can this (and the very similar logic above) be broken up to make it easier to digest please?

And maybe it can be consolidated. If I'm following the logic correctly, the only reason we can't simply check scope._futures_allowed here is that above, where its set in handleNode(), there's a possibility there was some from foo import bar or similar. That would have met the isinstance(node, ast.ImportFrom) condition, and so would not have set self.scope._futures_allowed = False

It would seem though that if we extend that condition just a bit, to match only from __future__ ..., then we can have the complete logic for determining if __future__ imports are allowed in just one place. What do you think?

[jayvdb]

The check for "any other binding in scope" because this method currently can not follow the old pattern

if node.module == '__future__':
   ...
else:
   self.futuresAllowed = False

"self.futuresAllowed = False" can not become "self.scope._futures_allowed = False" , because the current scope may not be a ModuleScope/DocScope, so "self.scope._futures_allowed" would be an attribute error.

We could work around that by using a more complex else clause like

if node.module == '__future__':
   ...
elif hasattr(self.scope, '_futures_allowed'):
   self.scope._futures_allowed = False

However that adds an extra branch to every 'import from' processed, whereas a "any other binding in scope" inside if node.module == '__future__' achieves the same result and is only executed within the specific __future__ import nodes, so it will only negatively impact performance when a module has many __future__ imports.

[bitglue]

I guess that makes sense, but if the code requires some explanation, then I'd like to improve the code so future readers won't run into the same difficulties I'm having understanding the code now.

What can be done to make this code more digestible? Can we:

• break up the compound conditional expressions into several if statements?
• assign intermediate names to some of the subexpressions?
• extract some of the logic to well-named private methods or functions?
• consolidate the logic?

[jayvdb]

I've added has_only_future_imports to name one aspect of it.

Except for that, it is the same as the previous logic, excepting that

not self.futureAllowed

is now

not isinstance(self.scope, ModuleScope) or not self.scope._futures_allowed .

I could name that compound conditional...

@property
def futuresAllowed(self):
    return not isinstance(self.scope, ModuleScope) or not self.scope._futures_allowed

;-)

[bitglue]

would adding _futures_allowed = False to the Scope base class allow the isinstance(self.scope, ModuleScope) to be dropped from this logic?

[jayvdb]

So, now the logic is wrapped up in the futuresAllowed property, and the code which disables futuresAllowed is not modified at all.
Are you comfortable with this?

[jayvdb]

this can be made more efficient, and simpler.

also need to re-add docstrings, and comments.

[bitglue]