Add DistroVersion class to compare distro versions #3078
Conversation
narrieta
requested review from
ZhidongPeng,
nagworld9 and
maddieford
as code owners
| @@ -68,23 +67,6 @@ def test_remove_bom(self): | |||
| data = textutil.remove_bom(data) | |||
| self.assertEqual(u" ", data) | |||
|
|
|||
| def test_version_compare(self): | |||
There was a problem hiding this comment.
moved to test_distro_version.py
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3078 +/- ##
===========================================
+ Coverage 71.62% 71.91% +0.28%
===========================================
Files 110 110
Lines 16433 16395 -38
Branches 2352 2342 -10
===========================================
+ Hits 11770 11790 +20
+ Misses 4112 4054 -58
Partials 551 551 ☔ View full report in Codecov by Sentry. |
|
|
||
| # AttributeError: 'FlexibleVersion' object has no attribute '_fragments' | ||
| with self.assertRaises(AttributeError): | ||
| _ = DistroVersion("1.0.0.0") == FlexibleVersion("1.0.0.0") |
There was a problem hiding this comment.
you mean it will fail with Attribute error If I use FlexibleVersion in the code. I didn't this comment
There was a problem hiding this comment.
this comparison raises AttributeError. the comment describes the message in the exception
| self.assertTrue(DistroVersion("FFFF") < DistroVersion("h")) | ||
| self.assertTrue(DistroVersion("None") < DistroVersion("n/a")) | ||
|
|
||
| if sys.version_info[0] == 2: |
There was a problem hiding this comment.
That means in prod there a chance that code can hit typeerror and break the agent?
There was a problem hiding this comment.
yes, the current code with LooseVersion has this issue
| self.assertTrue(DistroVersion("1.0.0") == DistroVersion("1.0.0")) | ||
| self.assertTrue(DistroVersion("1.0.0") != DistroVersion("2.0.0")) | ||
|
|
||
| self.assertTrue(DistroVersion("13") != DistroVersion("13.0")) |
There was a problem hiding this comment.
This looks to me DistroVersion is also variant of strict version and Flexible version also documented as same. Is that means both does same thing?
13 != 13.0 !=13.0.0 is true in both modules?
There was a problem hiding this comment.
DistroVersion is not a variant of StrictVersion, but of LooseVersion
There was a problem hiding this comment.
13 != 13.0 !=13.0.0 is true in both modules?
FlexibleVersion implements the comparison as expected:
>>> FlexibleVersion("13") == FlexibleVersion("13.0")
True
>>> FlexibleVersion("13") == FlexibleVersion("13.0.0")
True
>>> FlexibleVersion("13") == FlexibleVersion("13.0.1")
False
A previous PR added a copy of distuls/version.py as a temporary workaround to use LooseVersion (since it is deprecated and will be removed from Python). This PR replaces that copy with a new class: DistroVersion that implements the same behavior in terms of comparing versions.
The results of some of the comparisons done with LooseVersion/DistroVersion are not always intuitive and can lead to subtle code bugs (e.g. "1" != "1.0" != "1.0.0", etc). There are a couple of places in the code where we use similar comparisons; be careful with those.
Also, we have another class that deals with versions, FlexibleVersion, with its own behavior. This creates confusion as to what class to use when comparing versions.
I'll do another iteration of this code to merge DistroVersion and FlexibleVersion.