strip non-numeric/non-dot characters from version string #5944
Conversation
| assert isinstancemethod(MyClass, MyClass.an_instancemethod) | ||
|
|
||
|
|
||
| def _minversion_test(): |
There was a problem hiding this comment.
Why can't this just be part of test_minversion()?
There was a problem hiding this comment.
To avoid code repetition when both pkg_resources.parse_version and distutils.version.LooseVersion are tested.
There was a problem hiding this comment.
So you still plan to add more tests to this PR?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, yes. I see that now. Thanks for the clarification! 😄
|
This also needs a change log for v2.0 under "API change". |
Done (not an expert on rst, though). Let me know whether this is good and/or the commits need squashing. |
CHANGES.rst
Outdated
|
|
||
| - ``astropy.utils`` | ||
|
|
||
| - On systems that do not have ``pkg_resources`` non-numerical additions to |
There was a problem hiding this comment.
That's not an API change, right? It seems more like a bugfix. Not sure though, I generally struggle with these classifications. 😄
There was a problem hiding this comment.
Yes, you are right. I did tag the original issue as a bug two months ago. So, please move this to v1.3.3 under "Bug Fix". Sorry for the inconvenience.
| 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. | ||
| expr = '^([1-9]\\d*!)?(0|[1-9]\\d*)(\\.(0|[1-9]\\d*))*' |
There was a problem hiding this comment.
this seems overly complicated, e.g why not have [0-9] instead of 0|[1-9] or \d?
There was a problem hiding this comment.
The regex is lifted straight from PEP440, which defines valid version numbers. I had the same question as you when I read it but decided to go with the "official" version in case I had missed something. My brain's regex parser is notoriously unreliable.
There was a problem hiding this comment.
In the comment, adding a link to where that is defined in PEP440 would be nice.
|
I think it would be a good idea to squash the commits because it's a fairly small change and most commits are 1-2 lines changed. However let's wait if someone else thinks different before doing so (currently it's easy to see/review what changed in each commit :) ). |
|
I would recommend a squash and let the CI run one more time. |
|
Squashed and all green. |
|
Thanks again @joergdietrich! |
strip non-numeric/non-dot characters from version string
4 participants
Strip non-numeric version name extensions like
'dev'orrc1from version string ifLooseVersion()is used for version comparison. This fixes #5814 and adds a first test suite forminversion().