DM-15515 Enable flake8 tests. #22
Conversation
|
You will need to add a simple test file to |
jeffcarlin
force-pushed
the
tickets/DM-15515
branch
from
5ebe65a
to
cf6320d
Compare
examples/demoPool.py
Outdated
| @@ -8,12 +8,11 @@ | |||
| # | |||
|
|
|||
| from __future__ import print_function | |||
| from builtins import map | |||
| import math | |||
| from builtins import range | |||
There was a problem hiding this comment.
You can remove the builtins imports and the __future__ while you are here. We don't need py2 compatibility.
There was a problem hiding this comment.
Removed builtins and __future__ from all files in this repo.
python/lsst/ctrl/pool/log.py
Outdated
| import lsst.log as lsstLog | ||
| from lsst.utils import getPackageDir | ||
| standard_library.install_aliases() |
| @@ -15,7 +15,6 @@ | |||
| import contextlib | |||
| from lsst.pipe.base import CmdLineTask, TaskRunner | |||
| from .pool import startPool, Pool, NODE, abortOnError, setBatchType | |||
| from . import log # register pickle functions for log | |||
There was a problem hiding this comment.
The comment here suggests that this import has to happen even if log is not used. I suggest a # noqa here instead.
There was a problem hiding this comment.
I didn't see this line in parallel.py? Not sure where it went. Added it back in as "from . import log # noqa
There was a problem hiding this comment.
Also - is this import unnecessary? I get this:
================================================================ FAILURES =================================================================
________________________________________ FLAKE8-check(ignoring E133 E226 E228 E251 N802 N803 N806) ________________________________________
/home/jcarlin/repos/ctrl_pool/python/lsst/ctrl/pool/parallel.py:64:5: F811 redefinition of unused 'log' from line 15
There was a problem hiding this comment.
This is importing lsst.ctrl.pool.log. The problem is that in line 64 the code assigns a variable to log and that is triggering the warning because at that point the original log is lost. This is a tricky one to fix since people expect the logging system to be in log. Maybe change this import to
from . import log as dummyLog # noqa(I think you need two spaces before the #).
There was a problem hiding this comment.
Adopted your suggestion. (Not sure I understand why that import is needed, but trust your wisdom.)
There was a problem hiding this comment.
it's needed because there is code in ctrl.pool.log that needs to be run even if you don't use it directly (it enables pickling). Not sure why that couldn't be in the __init__.py though.
python/lsst/ctrl/pool/pool.py
Outdated
| @@ -1,8 +1,6 @@ | |||
| from future import standard_library | |||
python/lsst/ctrl/pool/pool.py
Outdated
| from builtins import zip | ||
| from builtins import range | ||
| from past.builtins import basestring | ||
| from builtins import object |
setup.cfg
Outdated
|
|
||
| [tool:pytest] | ||
| addopts = --flake8 | ||
| flake8-ignore = E133 E226 E228 E251 N802 N803 N806 |
There was a problem hiding this comment.
You are ignoring one extra code here. This can be confusing because it means that the pytest check is different to the pull request check. I don't think we use E251 do we?
There was a problem hiding this comment.
We were wondering about that. We got this setup from J. Sick, and noticed the extra there. According to https://developer.lsst.io/python/style.html?highlight=flake8#flake8-configuration-files we don't use E251.
There was a problem hiding this comment.
Removed E251 from `setup.cfg'.
jeffcarlin
force-pushed
the
tickets/DM-15515
branch
from
bb3a50e
to
f48a7a0
Compare
No description provided.