update pyflakes to 1.1.0, enable pyflakes checks and fix all warnings

Review Request #3601 — Created March 24, 2016 and submitted

cheister
pants
enable-pyflakes
3077
pants-reviews
gmalmquist, jsirois, nhoward_tw, patricklaw, stuhood, zundel
update pyflakes to 1.1.0, enable pyflakes checks and fix all warnings

Travis CI: https://travis-ci.org/pantsbuild/pants/builds/118569830

Enable PyFlakes https://pypi.python.org/pypi/pyflakes which claims: "it will never complain about style, and it will try very, very hard to never emit false positives."

This change enables all of the pyflakes checks documented at http://flake8.readthedocs.org/en/latest/warnings.html.

By far most of the fixes in this change are for unused imports but it did find several tests that had duplicate names, several unused variables and variables that were not defined.

  • 0
  • 0
  • 4
  • 1
  • 5
Description From Last Updated
  1. 5 pages of fixes!

    I really love that this is catching so many problems in exception handling code. That's the worst experiences for users of code written in a dynamic language - when something goes wrong in a new way you may be the first person to ever execute that code path.

    Turning this on may give headaches while we are developing code, so we'll have to gauge sentiment to see if this is too painful for day to day development. Looking at all the issues it found though, think it is worthwhile.

  2. I think the target name is important here. I think you could use 'library.address.sepc'

  3. I think they actually meant to print diagnostics for both stack_args and cmd_args?

  4. Was this a warning that we were shadowing 'coord' in another scope? Looks like itw as harmless in this case, but still a good fix I guess.

    1. I wrote this code originally; the shadowing here and in the place below is completely harmless, but making it explicit certainly doesn't hurt.

  5. src/python/pants/bin/BUILD (Diff revision 1)
     
     

    Note to other reviewers, this is a signficant change: the contrib module is optional, but we are using it as an integral part of the pants that we use to build locally to test pants. If we don't put it in, pyflakes fails to resolve in CI.

  6. yikes, this method was defined twice in this class. I guess this one was just ignored?

  7. @nhoward_tw: I'm guessing that this is leftover code before some refactoring of this test - the _test_canonical_classpath_helper I think sufficiently tests this by passing an exact list of jars it expects to find.

    1. I think this was intended to be added to the product, but wasn't. Without that, the excludes behavior isn't exercised, which I think was the intent of creating it. @peiyu would know for sure.

  8. 
      
  1. Two followups
    This is of course now begging for a matching BUILD file cleanup. I won't insist on you doing that by hand - but it might not take too long. Either way - it is another spur in my side about open sourcing buildgen.
    I could be way behind - but now we will use pylint, isort and pyflakes, right?. Can we combine or deprecate some of these linting steps? I see something called flake8 that looks to combine isort and pyflakes. The pylint chroots are really overkill imo - the same python files are linted over and over, even when they don't change.

    1. So. That comment is unreadable. Let's try to list my questions again:

      • This is of course now begging for a matching BUILD file cleanup. I won't insist on you doing that by hand - but it might not take too long. Either way - it is another spur in my side about open sourcing buildgen.
      • I could be way behind - but now we will use pylint, isort and pyflakes, right?. Can we combine or deprecate some of these linting steps? I see something called flake8 that looks to combine isort and pyflakes. The pylint chroots are really overkill imo - the same python files are linted over and over, even when they don't change.
    2. Are you suggesting running PyFlakes against the BUILD files and cleaning up warnings? or using something else to cleanup the BUILD files?

      There is flake8 https://pypi.python.org/pypi/flake8 which combines PyFlakes and PEP8 checks. Currently we aren't running any PEP8 checks against the repo though so switching to flake8 would be the same as just using the PyFlakes.

    3. AFAIK, cleaning up unused deps in BUILD files is currently a manual process. You essentially compare the list of dependencies in the target to the list of imports in each file.

      With buildgen for scala (a 4sq internal plugin), there is automatic analysis and a procedure to mechanically update BUILD files with the right dependencies.

  2. Yes - I would like to keep the library name - the android resolve pulls in the world of transitive deps so it would be useful debugging to get at least an idea of where to apply a fix.

    I think that library.address.spec instead of target would be right. Thanks!

  3. I don't have great insight into this, but this method may not be safe to remove - the docstring says "Returns the read cache for this setup, creating it if necessary." so it may be the creation of the cache that was in mind.

  4. 
      
  1. Thanks for all the fixes! I think these warnings will be helpful.

  2. I think this should be state_key.exc

  3. Did these tests maybe intend to actually use tempdir as the root directory or these files instead of hardcoding /tmp? (@zundel?)

    1. Everything else in the test is hardcoded to /tmp. Seems like maybe the whole test should be moved over to using tempdir? But maybe not in this PR?

    2. The reason for hard coding /tmp is that this path is in a BUILD file and the paths are passed on the command line to other processes. To fix it we would either need to:
      - Dynanamically set a symbol in a build file (say use string interpolation on buildroot)
      - Do something to generate the BUILD file.

  4. 
      
  1. Ship It!
  2. 
      
  1. Looks good!

  2. xx. I think this is a noop.

    1. I had actually removed this in the first rev but @mateor had a concern that the get_write_cache() call might have been there to initialize the cache. See comment above.

    2. Did you try removing it and seeing if it caused failures?

  3. 
      
  1. Looks like this is ready to merge in, but there were conflicts. @cheister could you rebase this change and submit it to CI? I can try to land it this weekend

  2. 
      
  1. Thanks Chris for this patch and Mateo, Stu, Nick, Garrett for the reviews. Commit 7571a52. Please close the associated github PRs and mark this review as submitted.

  2. 
      
Review request changed

Status: Closed (submitted)

Loading...