-
-
Notifications
You must be signed in to change notification settings - Fork 767
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
Stabilize python default version lookup #1117
Conversation
An effect of the instability is that pre-commit will end up installing dependencies several times in separate redundant virtualenvs over multiple runs, depending on which version happened to be returned. |
For example, for sys.executable: /usr/bin/python3 -> python3.7 ...the default lookup may return either python3 or python3.7. Make the order deterministic by iterating over tuple, not set, of candidates.
Don't know what's up with the tests, they pass for me in local tox on Linux. |
as written, those tests require everything to be on they pass locally for you because you have all of those pythons available patch looks fine -- even a patch which just changes the brackets would probably be fine to merge if this is difficult to test |
tests/languages/python_test.py
Outdated
with mock.patch.object(sys, 'executable', exe): | ||
with mock.patch('os.path.realpath', return_value=realpath): | ||
assert python._find_by_sys_executable() == expected | ||
with mock.patch('pre_commit.parse_shebang.find_executable', |
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.
please use mock.patch.object
instead, mock.patch
has import side-effects and can lead to some nasty patch leaking issues. mock.patch.object
is also more explicit about the target
also, that feel when you don't run the pre-commit hooks while contributing to pre-commit 😆
tests/languages/python_test.py
Outdated
|
||
|
||
@pytest.mark.parametrize( | ||
'exe,realpath,expected', ( |
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.
would prefer a tuple of strings here instead of stringly typed parametrize as well
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.
realized I can do a few of the nitpicks as suggestions
pre_commit/languages/python.py
Outdated
|
||
|
||
def _get_default_version(): # pragma: no cover (platform dependent) | ||
|
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.
tests/languages/python_test.py
Outdated
|
||
|
||
@pytest.mark.parametrize( | ||
'exe,realpath,expected', ( |
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.
'exe,realpath,expected', ( | |
('exe', 'realpath', 'expected'), ( |
Thanks, learned something new :) And I hope I caught all of it now. |
tests/languages/python_test.py
Outdated
@@ -7,6 +7,7 @@ | |||
import mock | |||
import pytest | |||
|
|||
import pre_commit.parse_shebang |
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.
import pre_commit.parse_shebang | |
from pre_commit import parse_shebang |
tests/languages/python_test.py
Outdated
with mock.patch.object(sys, 'executable', exe): | ||
with mock.patch.object(os.path, 'realpath', return_value=realpath): | ||
with mock.patch.object( | ||
pre_commit.parse_shebang, 'find_executable', |
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.
pre_commit.parse_shebang, 'find_executable', | |
parse_shebang, 'find_executable', |
Damn, still not quite there it seems :/ |
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 example, for sys.executable:
...the default lookup may return either
python3
orpython3.7
. Make the order deterministic by iterating over tuple, not set, of candidates.