Minor changes for #1065 and #1044 #1089
Conversation
| asv/environment.py, | ||
| asv/machine.py, | ||
| asv/graph.py, | ||
| asv/extern, | ||
| asv/plugins, | ||
| asv/template/benchmarks, |
There was a problem hiding this comment.
I don't think we've got issues for everything that is pending. Can you check and make sure we've got issues to have the whole repo styles. Either by opening new issues, opening PRs for the parts without issues, or a mix. Thanks!
There was a problem hiding this comment.
asv/plugins/virtualenv.py
Outdated
| @@ -48,7 +48,7 @@ def __init__(self, conf, python, requirements, tagged_env_vars): | |||
| tagged_env_vars) | |||
|
|
|||
| try: | |||
| import virtualenv # noqa F401 unused, but required in case the previous step does not work | |||
| import virtualenv # noqa F401 unused, but is needed even if not used | |||
There was a problem hiding this comment.
I think what you're saying here doesn't add much value. It's clearly needed if unsused, that's why we have the noqa. Would be more useful to explain why is needed.
There was a problem hiding this comment.
Sorry, Marc, I did not get the idea. Could you please clarify a little bit more?
There was a problem hiding this comment.
This line has an import, which is unused. We don't want to have unused imports in general, there is no point. In this case we're going to leave it, and we're telling flake8 this is fine, we want to leave an unused import. That's fine, but let readers of this code know why we're leaving this import. And saying "because it's needed" it's like not saying anyting. Please add here a comment on why this import is needed. In this case it's quite obvious, and could not even be needed, but a short sentence to help readers would still be nice.
LucyJimenez
changed the title
setup.cfg
Outdated
| @@ -4,12 +4,11 @@ ignore = | |||
| W504, # W504: Line break occurred after a binary operator | |||
| E741 # E741: Do not use variables named 'I', 'O', or 'l' | |||
| exclude = setup.py, | |||
| asv/commands, | |||
| asv/benchmarks, | |||
There was a problem hiding this comment.
Why are we reintroducing this to the excludes?
There was a problem hiding this comment.
Sorry, it was a mistake.
asv/plugins/virtualenv.py
Outdated
| @@ -48,7 +48,7 @@ def __init__(self, conf, python, requirements, tagged_env_vars): | |||
| tagged_env_vars) | |||
|
|
|||
| try: | |||
| import virtualenv # noqa F401 unused, but required in case the previous step does not work | |||
| import virtualenv # noqa F401 unused, but required to manage an environment | |||
There was a problem hiding this comment.
This is not required to manage any environment. This import is just used to test whether virtualenv is installed or not, and its goal is to fail here with a useful error message (instead of elsewhere I guess) if the package is not installed.
There was a problem hiding this comment.
Great, thank you so much for the explanation!
|
Thanks @LucyJimenez |
Closes #1065 #1044
Minor changes:
asv/plugins/virtualenv.pyasv/plugins,asv/commands