-
Notifications
You must be signed in to change notification settings - Fork 532
FIX: Make vendored LooseVersion comparable to distutils.version.LooseVersion #3466
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
Conversation
197ea62
to
aa99263
Compare
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.
LGTM as is, but 1 comment
@pytest.mark.parametrize("v1, v2", [("0.0.0", "0.0.0"), ("0.0.0", "1.0.0")]) | ||
def test_LooseVersion_compat(v1, v2): | ||
vend1, vend2 = Vendored(v1), Vendored(v2) | ||
orig1, orig2 = Original(v1), Original(v2) |
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.
should we catch this DeprecationWarning
warning that occurs when initializing a distutils Version?
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.
Do you get one? I don't in 3.10.
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.
yeah, and i'm on 3.10.4...
DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
v = LooseVersion("0.0")
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.
Interesting. I'm on 3.10.4 and I don't see that. Does it happen just on the first one?
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.
$ pytest nipype/external/tests/test_version.py
==================================================================== test session starts =====================================================================
platform darwin -- Python 3.10.4, pytest-7.1.1, pluggy-1.0.0
rootdir: /Users/mathiasg/code/nipype/nipype, configfile: pytest.ini
collected 2 items
nipype/external/tests/test_version.py .. [100%]
====================================================================== warnings summary ======================================================================
nipype/interfaces/io.py:1406
/Users/mathiasg/code/nipype/nipype/interfaces/io.py:1406: DeprecationWarning: invalid escape sequence '\w'
field_name = re.match("\w+", field_name).group()
nipype/interfaces/utility/base.py:232
/Users/mathiasg/code/nipype/nipype/interfaces/utility/base.py:232: DeprecationWarning: invalid escape sequence '\w'
"""Change the name of a file based on a mapped format string.
../../.pyenv/versions/3.10.4/envs/nipreps/lib/python3.10/site-packages/_pytest/config/__init__.py:1252
/Users/mathiasg/.pyenv/versions/3.10.4/envs/nipreps/lib/python3.10/site-packages/_pytest/config/__init__.py:1252: PytestConfigWarning: Unknown config option: env
self._warn_or_fail_if_strict(f"Unknown config option: {key}\n")
external/tests/test_version.py::test_LooseVersion_compat[0.0.0-0.0.0]
external/tests/test_version.py::test_LooseVersion_compat[0.0.0-0.0.0]
external/tests/test_version.py::test_LooseVersion_compat[0.0.0-1.0.0]
external/tests/test_version.py::test_LooseVersion_compat[0.0.0-1.0.0]
/Users/mathiasg/code/nipype/nipype/external/tests/test_version.py:18: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
orig1, orig2 = Original(v1), Original(v2)
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
in any case, it's not a big deal
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.
Okay. I'll wrap it. Thanks for confirming.
Codecov Report
@@ Coverage Diff @@
## master #3466 +/- ##
=======================================
Coverage 65.25% 65.25%
=======================================
Files 309 309
Lines 40838 40838
Branches 5376 5376
=======================================
Hits 26647 26647
Misses 13117 13117
Partials 1074 1074
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
Turns out we need LooseVersions to be compatible with one another.