Fix flake8 violations#1029
Conversation
datapythonista
left a comment
datapythonista
left a comment
There was a problem hiding this comment.
Looks good, just couple of things that we should revert. Thanks!
|
@datapythonista this PR is ready for your review |
test/test_results.py
Outdated
| import pytest | ||
|
|
||
| from .tools import example_results | ||
| from .tools import example_results # noqa :F401 imported but unused |
There was a problem hiding this comment.
For the comment, we don't want the flake8 error, which doesn't add much value. What we want is to explain why we are ignoring the flake8 error. Reading the comment, the obvious thing to think is that this import should be deleted, since it's imported but unused. A helpful comment would be to say that this is required even if unused, so the example_results fixture is loaded.
For this particular case, what it may make more sense is to move the fixtures from tools.py to the standard conftest.py. This way this import won't be needed, as pytest will take care of everything. We don't want to have such a change in a PR with minor style changes. So, one option would be to:
- Change this line to something like
from . import tools # noqa F401 needed to load fixtures (see #1234) - Remove the other
noqabecause of redefinition ofexample_results, which shouldn't be needed anymore - Open an issue to explain what's going on here, to move the fixtures to the right location
There was a problem hiding this comment.
Thank you @datapythonista
for your review let me incorporate these changes
There was a problem hiding this comment.
Hello @datapythonista this still happens when i remove the # noqa
test/test_results.py:165:31: F811 redefinition of unused 'example_results' from line 17
test/test_results.py:208:39: F811 redefinition of unused 'example_results' from line 17
There was a problem hiding this comment.
I left a new review explaining how I would fix this. You need the noqa here with the proposed import, but not the errors you have in your comment I think.
test/tools.py
Outdated
|
|
||
| try: | ||
| import virtualenv | ||
| import virtualenv # noqa :F401 imported but unused |
There was a problem hiding this comment.
Same, this comment doesn't add any value, what we want is to explain why we keep this import even if it's unused, not just the flake8 error message, which doesn't provide any value to the readers of this
test/test_results.py
Outdated
|
|
||
|
|
||
| def test_backward_compat_load(example_results): | ||
| def test_backward_compat_load(example_results): # noqa uses fixture |
There was a problem hiding this comment.
Can you ignore only the error code we have now.
test/test_results.py
Outdated
|
|
||
|
|
||
| def test_iter_results(capsys, tmpdir, example_results): | ||
| def test_iter_results(capsys, tmpdir, example_results): # noqa uses fixture |
test/test_results.py
Outdated
|
|
||
|
|
||
| def test_backward_compat_load(example_results): # noqa uses fixture | ||
| def test_backward_compat_load(example_results): # noqa F811 |
There was a problem hiding this comment.
Can you add the comment on why we are ignoring this error please
test/test_results.py
Outdated
|
|
||
|
|
||
| def test_iter_results(capsys, tmpdir, example_results): # noqa uses fixture | ||
| def test_iter_results(capsys, tmpdir, example_results): # noqa F811 |
|
Thanks @dorothykiz1, very nice |
2 participants
Closes 15
Fix flake8 violations in files below;