Install pyflakes on travis

[jchodera]

Addresses #121

[jchodera]

@peastman: The tests work now, but you still have a test failure to address.

[peastman]

Something strange happened with travis. The Python 2.7 and 3.4 builds didn't see the changes from my last PR. The Python 3.5 build did see them, and all tests passed on that one. However, pyflakes reported a failure. But the things it's complaining about aren't really errors, just unused imports and things like that.

[jchodera]

However, pyflakes reported a failure. But the things it's complaining about aren't really errors, just unused imports and things like that.

The pyflakes page says "If you require more options and more flexibility, you could give a look to Flake8 too."

You might want to use Flake8 if you want to tell it not to fail when you want to configure it to ignore some warnings or or errors.

[peastman]

I went through the pyflakes warnings, and most of them are legitimate. I'll send a PR fixing them. They aren't really problems, just unused code, but it's still good to clean things like that up.

There was one case where it was wrong, though. It complained about this line in __init__.py:

from .pdbfixer import PDBFixer

If that file were executed in isolation, then yes, this would be an unused import. But it isn't. I'd call this a bug in pyflake. It should never report "unused" imports or definitions in __init__.py files, because they may be used by other code that imports the module. I'll look into flake8 to see if it can handle this better. Or we could modify the travis script to skip that file.

[jchodera]

If that file were executed in isolation, then yes, this would be an unused import. But it isn't. I'd call this a bug in pyflake.

Have you tried Flake8? It may have solved this problem.

We typically don't use pyflakes to do automated testing on our other python codes, so I don't have any more insight here.

[peastman]

It doesn't look like that will help. It lets you exclude whole classes of errors or warnings, so you could have it omit warnings about unused imports in all files. But it just wraps pyflakes, so it still doesn't understand that some unused imports are actually legitimate.

I think it's best to just remove pyflakes from travis. It's still a handy tool to run on our own now and then, but if it sometimes marks correct code as incorrect, that shouldn't cause travis to mark the build as failing.

[swails]

There was one case where it was wrong, though. It complained about this line in __init__.py:

from .pdbfixer import PDBFixer

If that file were executed in isolation, then yes, this would be an unused import. But it isn't. I'd call this a bug in pyflake. It should never report "unused" imports or definitions in init.py files, because they may be used by other code that imports the module. I'll look into flake8 to see if it can handle this better. Or we could modify the travis script to skip that file.

It's subtle, but this is actually intentional behavior in pyflakes. There's an easy way to fix it -- add an __all__ attribute to these files. This is good practice to do, anyway, since it explicitly defines the behavior of from package import * (it's already well-defined, but __all__ prevents namespace pollution in principle) and allows introspective tools that rely on it to work.

I run pyflakes as part of the standard ParmEd test suite and am pretty pleased with it overall. Unfortunately I bundle enough 3rd-party packages that I don't want to change that I need to filter out warnings coming from those packages, but for my own code I'm pretty strict about adhering to their warnings.

[swails]

Just to be explicit, nothing that appears in __all__ will ever be reported as "unused" by pyflakes. And a blanket statement applying to all of __init__.py doesn't really fly, either, since code refactoring happens there, too, and unused imports should be cleaned up when left behind. I think using __all__ is the right way to go here.

[peastman]

Thanks, that's good to know.