Always report imports shadowed by another import

[jayvdb]

Previously only unused imports redefined by another
imports were reported as RedefinedWhileUnused.

A new ImportShadowedByImport message now always
reports the re-import except when an import in the
doctest scope shadows an import in the module scope.

[bitglue]

What's the use of the *(...) construction here?

[jayvdb]

To simplify neater indentation.

[sigmavirus24]

Eh, this isn't exactly clear or obvious. I understand why, but I'm not sure I agree with it.

[jayvdb]

No worries; I can rework it when I create a narrower patch after #54 has been approved.

[jayvdb]

Yea, I was going to note that this and #44 should not both be merged without amendments, but since you require fast-forward merges only that note isnt necessary.

[bitglue]

Sorry I haven't been keeping up with the PRs lately. I'm busy with work stuff this week but I'll try to take a look at these next week. Alternately, I'm fine with any of these changes being merged if anyone else reviews them first.

[ambv]

Similarly to #55, I would use this but it doesn't belong to pyflakes on the basis of not reporting an actual error. A superfluous import is absolutely harmless and a local, alternative import is useful (even if constitutes questionable style).

I do agree unused imports break the "errors only" rule, too, but this is historical and it seems to me it would open a Pandora box of other warning-type suggestions if I let this one in. It really seems we need a flake8-likely-bugs plugin that sits between what pyflakes tests for (hard errors) and what pep8 does (pure style suggestions).

[bitglue]

Superfluous imports aren't harmless: they have (sometimes significant) performance consequences and create edges in the dependency graph that are unnecessary which can lead to dependency cycles that cause imports to fail in confusing and mysterious ways.

In fact this very problem was the reason pyflakes was written in the first place.

I think you miss the point of pyflakes. It is not to catch "hard errors". That's what unit tests are for. It's to catch "soft errors" which would not be caught by a unit test, but only actual errors, not maybe errors.

There is never any reason to import something and not use it, with one exception: importing stuff into init.py or similar to make an ubermodule that consolidates the namespaces of multiple submodules. And this is why we have __all__.

[asmeurer]

There are use-cases (like when a module import doesn't exist until you import some other thing, like https://twitter.com/asmeurer/status/468514695154921472), but they are very rare, and I think generally can be worked around. I agree the unused imports, while they seem harmless, are actually useful, in the same way that unused variables are useful. If you imported something and don't use it, that often indicates that you forgot something (not as often as importing a variable and not using it, but often enough). I've also had pyflakes tell me that I'm not using an external module any more, meaning my code had an unnecessary unused dependency from an unused import. And unused imports are trivial to fix, and I doubt anyone will argue against doing so for some style reason (contrast this with some pep8 warnings, some of which I strongly disagree with). And FWIW I disagree that you should have to define __all__ in __init__.py for pyflakes to not warn about unused imports/names.

[ambv]

@bitglue, by a superfluous import I mean the exact case covered by this pull request, e.g. an import done within the function scope, when a module-level import has already been made. These have almost zero runtime penalty as they're reusing modules already present in sys.modules.