Complain about unused imports only when the features are used

[terite]

Don't complain all the time about print, division, etc. Instead, just
complain when the module in question actually uses a language feature
that would be affected.

Closes #1, and may address #10

This includes the commits from #12 just to get that nice green check mark. Nightly doesn't pass, but I'm not sure how legitimate that failure is, especially considering that 3.6 doesn't even run.

[terite]

I do these checks inside of generic_visit because for this file:

print 'foo'

That should trigger both uses_str_literals and uses_print, but if I define visit_Print and visit_Str, it seems only one was ever getting called.

[xZise]

That is because you need to call the super's method like super(…).visit_Print().

[terite]

Unfortunately, I get AttributeError: 'super' object has no attribute 'visit_Print'

http://hastebin.com/evagoziduh.py

[terite]

I need some help with the existing tests. There's a ton of stuff going on in them, and I don't find them intuitive at all.

[xZise]

In general the code looks fine. I don't have really an objection regarding using generic_visit. The main issue I have is that it never complains now about a missing import if it is not necessary. I for example wouldn't want that because I want to enforce a specific set of imports on all files even if they are not necessary there, but just to be consistent.

Regarding the tests, I looked at your Travis 2.7 build and I use the line numbers of the repo at that time. Your patch is adding one line so your lines will be at the moment one line lower. The lines 50-52 check that the imports reported don't mention any unknown future imports (51 and 52) and line 50 checks that it only mentions each import at most once (e.g. don't mention the same import as missing and present).

Then in line 61 and 62 (where you got errors in 2.7) it checks that it returns a complementary set of errors. There is a set A of available imports in Python (all_available) and then checks that the combination of the sets P (errors that an import is present) and M (errors that an import is missing) is the same set A. Basically that there is an error message for each import possible (via line 50 we already checked that P and M have no intersection).

Now your code is not always reporting all missing imports and so it'll fail in line 61 where it checks that the set M is equal to A - U (U are the imports used). And at the moment there is no test which is verifying that it can ignore codes (in which case M would be A - U - Im where Im is the set of ignored missing imports.

Now as I mention in the first paragraph I'd prefer if there is an option to modify this automatic and in that case the current tests could disable your changes and it should work. Alternatively you need to modify run_test or check_result in such a way that it expects that certain “missing errors” are missing because the code doesn't require them. You could also modify generate_code to actually require the imports (e.g. by doing division).

By the way the lines 142 and 142 works similar to the lines 61 and 62. By default it always expects all imports to be missing (expect for the test number 10 in which case it doesn't expect absolute_import and print_function) defined via expected_imports in lines 133 and 134.

I hope this helps you to identify what to change your PR to make it compatible :)

[terite]

@xZise thank you for your responses. I moved the compile call change to a separate PR.

To fix tests and the "it never complains now about a missing import if it is not necessary" problem, I chose to hide this feature behind a --require-used flag.

On the subject of tests, I'm in the Don't put logic in tests camp. There's a ton of logic and code, in addition to external dependencies. I tried to go a different direction for the tests I wrote. My hope is that future developers will be able to quickly and easily understand their purpose, without having to understand all the supporting pieces.

[terite]

Let me know if you'd like me to squash any of these commits.

[xZise]

Sorry for the late response. Now there are the following things:

• I'd prefer if you make it Python 2.6 compatible. That mainly means don't use {…} to create sets but set([…]).
• And yes could you squash your fixes into the previous two patches. That there is a second patch adding an option is fine.

Apart from that the code looks good so far. Unfortunately I may not be available over the next two months so it might be that I can't get it merged after that. But I just want to let you know so that you don't think I don't appreciate your patch. 👍

And I understand the reason why there shouldn't be logic in tests and maybe I have to force myself to follow that as well. But I personally can't stand when I need to copy code.

[jayvdb]

for absolute correctness, unicode_literals should only be required when there are string literals that are not prefixed ... i.e. if a module uses b'..' throughout, unicode_literals has no effect.

This may also need ast.FormattedValue and ast.JoinedStr, on Python 3.6 -- i havent tested these.

[xZise]

This should be possible using tokenize. Iterate over each string token and store it in a dictionary using the position and then you could here get that token from the dictionary (key = (node.lineno, node.col_offset)) and check the prefixes.

For a similar a script (flake8-string-format) I have already implemented a system like this to detect if a string is a raw string. And the main issue (and why I haven't released the other script already) is that it might be that there is no filename but that it's read from stdin.

[jayvdb]

Note the b'..' is taken care of by py3 as it has ast.Bytes. iirc, tokenize isnt necessary here for py2; just need to check the type of node.s .

[xZise]

There is no difference between '…' and b'…' in Python 2 (unless unicode_literals is used):

>>> ast.parse("b'bytes'\n'string'").body[0].value.s
'bytes'
>>> type(ast.parse("b'bytes'\n'string'").body[0].value.s)
<type 'str'>
>>> ast.parse("b'bytes'\n'string'").body[1].value.s
'string'
>>> type(ast.parse("b'bytes'\n'string'").body[1].value.s)
<type 'str'>

[xZise]

But with Flake8 3.x plugins can directly get the tokens, so it'd be possible to avoid any special “stdin” handling or such.

[jayvdb]

Nice. Maybe that part of the problem should be a separate patch after this one.

[codecov-io]

Codecov Report

Merging #3 into master will increase coverage by 1.76%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master       #3      +/-   ##
==========================================
+ Coverage   90.88%   92.65%   +1.76%     
==========================================
  Files           3        3              
  Lines         417      490      +73     
==========================================
+ Hits          379      454      +75     
+ Misses         38       36       -2

Impacted Files	Coverage Δ
test_flake8_future_import.py	96.81% <100%> (+0.83%)	⬆️
flake8_future_import.py	86.38% <100%> (+4.15%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f789cce...597e20e. Read the comment docs.

[terite]

I took another pass at this to get it compatible with the recent changes on master and flake8 3.x. I believe I've addressed the necessary feedback, but please let me know if there's anything I missed.

[meshy]

I'd really love to have this feature. Is there anything that can be done to move this forward?