Stabilize python default version lookup#1117
Stabilize python default version lookup#1117asottile merged 5 commits intopre-commit:masterfrom scop:py3-default-version
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 |
| 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.
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 😆
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| 'exe,realpath,expected', ( |
There was a problem hiding this comment.
would prefer a tuple of strings here instead of stringly typed parametrize as well
asottile
left a comment
There was a problem hiding this comment.
realized I can do a few of the nitpicks as suggestions
|
|
||
|
|
||
| def _get_default_version(): # pragma: no cover (platform dependent) | ||
|
|
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| 'exe,realpath,expected', ( |
There was a problem hiding this comment.
| 'exe,realpath,expected', ( | |
| ('exe', 'realpath', 'expected'), ( |
|
Thanks, learned something new :) And I hope I caught all of it now. |
| import mock | ||
| import pytest | ||
|
|
||
| import pre_commit.parse_shebang |
There was a problem hiding this comment.
| import pre_commit.parse_shebang | |
| from pre_commit import parse_shebang |
| 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.
| pre_commit.parse_shebang, 'find_executable', | |
| parse_shebang, 'find_executable', |
|
Damn, still not quite there it seems :/ |
For example, for sys.executable:
...the default lookup may return either
python3orpython3.7. Make the order deterministic by iterating over tuple, not set, of candidates.