Skip to content

strip non-numeric/non-dot characters from version string #5944

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.

Sign up for GitHub

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

Merged
bsipocz merged 1 commit into from
Apr 20, 2017
Merged

strip non-numeric/non-dot characters from version string #5944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGES.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,10 @@ Bug Fixes

- ``astropy.utils``

- On systems that do not have ``pkg_resources`` non-numerical additions to
version numbers like ``dev`` or ``rc1`` are stripped in ``minversion`` to
avoid a ``TypeError`` in ``distutils.version.LooseVersion`` [#5944]

- ``astropy.visualization``

- ``astropy.vo``
Expand Down
8 changes: 8 additions & 0 deletions astropy/utils/introspection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@


import inspect
import re
import types

from ..extern import six
Expand Down Expand Up @@ -154,6 +155,13 @@ def minversion(module, version, inclusive=True, version_path='__version__'):
from pkg_resources import parse_version
except ImportError:
from distutils.version import LooseVersion as parse_version
# LooseVersion raises a TypeError when strings like dev, rc1 are part
# of the version number. Match the dotted numbers only. Regex taken
# from PEP440, https://www.python.org/dev/peps/pep-0440/, Appendix B
expr = '^([1-9]\\d*!)?(0|[1-9]\\d*)(\\.(0|[1-9]\\d*))*'
m = re.match(expr, version)
if m:
version = m.group(0)

if inclusive:
return parse_version(have_version) >= parse_version(version)
Expand Down
25 changes: 24 additions & 1 deletion astropy/utils/tests/test_introspection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from ...tests.helper import pytest
from .. import introspection
from ..introspection import (find_current_module, find_mod_objs,
isinstancemethod)
isinstancemethod, minversion)


def test_pkg_finder():
Expand Down Expand Up @@ -92,3 +92,26 @@ def a_staticmethod(): pass
assert not isinstancemethod(MyClass, MyClass.another_classmethod)
assert not isinstancemethod(MyClass, MyClass.a_staticmethod)
assert isinstancemethod(MyClass, MyClass.an_instancemethod)


def _minversion_test():

@pllim pllim Apr 5, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't this just be part of test_minversion()?

@joergdietrich joergdietrich Apr 5, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid code repetition when both pkg_resources.parse_version and distutils.version.LooseVersion are tested.

@pllim pllim Apr 5, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you still plan to add more tests to this PR?

@joergdietrich joergdietrich Apr 5, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. _minversion_test() is called twice already, at least on systems that have pkg_resources. Once inside the if clause to trigger an ImportError if pkg_resources is present to fall back on distutils and once after the if clause to use pkg_resources.

@pllim pllim Apr 5, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes. I see that now. Thanks for the clarification! 😄

from types import ModuleType
test_module = ModuleType(str("test_module"))
test_module.__version__ = '0.12.2'
good_versions = ['0.12', '0.12.1', '0.12.0.dev']
bad_versions = ['1', '1.2rc1']
for version in good_versions:
assert minversion(test_module, version)
for version in bad_versions:
assert not minversion(test_module, version)


def test_minversion():
import sys
if 'pkg_resources' in sys.modules:
pkg_resources_saved = sys.modules['pkg_resources']
# Force ImportError for pkg_resources in minversion()
sys.modules['pkg_resource'] = None
_minversion_test()
sys.modules['pkg_resource'] = pkg_resources_saved
_minversion_test()