-
Notifications
You must be signed in to change notification settings - Fork 180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix flake8 violations #1029
Fix flake8 violations #1029
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just couple of things that we should revert. Thanks!
@datapythonista this PR is ready for your review |
test/test_results.py
Outdated
@@ -14,7 +14,7 @@ | |||
from asv import results, runner, util | |||
import pytest | |||
|
|||
from .tools import example_results | |||
from .tools import example_results # noqa :F401 imported but unused |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
noqa
because 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @datapythonista
for your review let me incorporate these changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
HAS_CONDA = False | ||
|
||
|
||
try: | ||
import virtualenv | ||
import virtualenv # noqa :F401 imported but unused |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -162,7 +161,7 @@ def test_get_result_hash_from_prefix(tmpdir): | |||
assert 'one of multiple commits' in str(excinfo.value) | |||
|
|||
|
|||
def test_backward_compat_load(example_results): | |||
def test_backward_compat_load(example_results): # noqa uses fixture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you ignore only the error code we have now.
test/test_results.py
Outdated
@@ -205,7 +204,7 @@ def test_json_timestamp(tmpdir): | |||
assert values['duration'] == duration | |||
|
|||
|
|||
def test_iter_results(capsys, tmpdir, example_results): | |||
def test_iter_results(capsys, tmpdir, example_results): # noqa uses fixture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
test/test_results.py
Outdated
@@ -161,7 +161,7 @@ def test_get_result_hash_from_prefix(tmpdir): | |||
assert 'one of multiple commits' in str(excinfo.value) | |||
|
|||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the comment on why we are ignoring this error please
test/test_results.py
Outdated
@@ -204,7 +204,7 @@ def test_json_timestamp(tmpdir): | |||
assert values['duration'] == duration | |||
|
|||
|
|||
def test_iter_results(capsys, tmpdir, example_results): # noqa uses fixture | |||
def test_iter_results(capsys, tmpdir, example_results): # noqa F811 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
Thanks @dorothykiz1, very nice |
Closes 15
Fix flake8 violations in files below;