Add support for version compare and for checking for version part argument #20

arcanefoam · 2018-05-10T09:24:28Z

Version compare is important for other scripts that use bumpversion as a library, e.g. in my big osgi project I want to make sure all projects are at the same version, find which ones are ahead/behind.

I also took another approach to checking the version part argument. My implementation checks against the current VersionConfiguration (not hardcoded to specific names).

Both changes have proper tests (I think) in place.

ekohl

This is just an superficial review. I didn't look into all the details yet.

ekohl · 2018-06-02T20:04:49Z

bumpversion/__init__.py

+        if not isinstance(other, Version):
+            return False
+        try:
+            return all(v == other[k] for k, v in self.items())


Doesn't this ignore the case there len(self.items()) < len(other.items())?

It does, we could do a len compare first to make sure both have the same number of parts. I will add this.

On a second thought, I think equality should be strict, that is: 1.4 != 1.4.0, by assuming they are the same we ignore the version pattern defined by parse and serialize. If both versions where generated by bumpversion then this highlights that the user has an error in the part configuration.

ekohl · 2018-06-02T20:05:53Z

bumpversion/__init__.py

+            for vals in ((v, other[k]) for k, v in self.items()):
+                if vals[0] == vals[1]:
+                    continue
+                if method(vals[0], vals[1]):


Can be return method(vals[0], vals[1]) but then I also prefer to rewrite it a bit:

if vals[0] != vals[1]: return method(vals[0], vals[1])

yes, cant really remember why did I do a if to return true/false.

ekohl · 2018-06-02T20:09:40Z

bumpversion/version_part.py

+        return hash(self.value)
+
+    def _compare(self, other, method):
+            if self.config.function_cls is not other.config.function_cls:


Odd indenting here

arcanefoam · 2018-07-13T08:43:36Z

Sorry for the delay on this. Have pushed the requested changes, except for the equals (see my comments).

ekohl

It took a while to get back to this. Some inline comments.

ekohl · 2018-08-01T14:36:47Z

bumpversion/__init__.py

    def __repr__(self):
-        return '<bumpversion.Version:{}>'.format(keyvaluestring(self._values))
+        by_part = ", ".join("{}={}".format(k, v) for k, v in self.items())
+        return '<bumpversion.Version:{}>'.format(by_part)


Probably cleaner to use self.__class__ to show the class name rather than hardcoding.

Makes sense. Do notice that this is how it was done in the past, I just added the by_part line ;)

ekohl · 2018-08-01T14:40:42Z

bumpversion/__init__.py

+        return not self.__eq__(other)
+
+    def __le__(self, other):
+        return self._compare(other, lambda s, o: o > s, False)  # Note the change of order in operands


You can also use operator.lt instead of the lambda. There's also operator.gt

ekohl · 2018-08-01T14:46:56Z

bumpversion/__init__.py

+    def __hash__(self):
+        return hash(tuple((k, v) for k, v in self.items()))
+
+    def _compare(self, other, method, strict=True):


This function is really hard to understand at first sight. The strict parameter should be named something clearer, perhaps allow_equal?

Indeed, I have changed the logic and improved the docs so the intent is more clear

ekohl · 2018-08-01T14:49:08Z

bumpversion/__init__.py

 class Version(object):

-    def __init__(self, values, original=None):
+    def __init__(self, values, config, original=None):


I'm not a fan of the config here. It doesn't feel very logical that a version has a config, especially if it's only used for order.

Agreed, will only use the order

ekohl · 2018-08-01T14:49:48Z

bumpversion/__init__.py

-    def __init__(self, values, original=None):
+    def __init__(self, values, config, original=None):
        self._values = dict(values)
+        self.config = config


This is IMHO an internal property so should be named self._config

Config is now the order and it has to be public because when comparing we first compare the orders cause if they are different the versions can not be comapred

ekohl · 2018-08-01T14:50:41Z

bumpversion/version_part.py

+
    def __eq__(self, other):
-        return self.value == other.value
+        return self._compare(other, lambda s, o: s == o)


Have a look at the operator module here as well.

ekohl · 2018-08-01T14:52:19Z

bumpversion/version_part.py

+        return self._compare(other, lambda s, o: s > o)
+
+    def __ne__(self, other):
+        return self._compare(other, lambda s, o: s != o)


Can this be simplified to return not self == other? See https://stackoverflow.com/questions/4352244/python-should-i-implement-ne-operator-based-on-eq#30676267 as well.

ekohl · 2018-08-01T14:56:30Z

bumpversion/__init__.py

+        try:
+            for vals in ((v, other[k]) for k, v in self.items()):
+                if vals[0] == vals[1]:
+                    continue


IMHO it's cleaner to use avoid continue

See previous comment on this method

On a second pass I can not find a way to avoid the continue :/

@arcanefoam Since the only purpose of the continue is to prevent calling return, you can just invert the logic. Instead of

for vals in ((v, other[k]) for k, v in self.items()): if vals[0] == vals[1]: continue return method(vals[0], vals[1])

Do

for vals in ((v, other[k]) for k, v in self.items()): if vals[0] != vals[1]: return method(vals[0], vals[1])

I would also improve the readability by changing this to

for key, value in self.items(): other_value = other[key] if value != other_value: return method(value, other_value)

ekohl · 2018-08-01T14:57:30Z

bumpversion/__init__.py

+            raise TypeError("Versions use different parts, cant compare them.")
+
+    def __ne__(self, other):
+        return not self.__eq__(other)


https://stackoverflow.com/questions/4352244/python-should-i-implement-ne-operator-based-on-eq#30676267 recommends avoiding this but rather write it as return not self == other. This is the default in Python 3 (but not 2).

c4urself · 2018-11-18T23:42:32Z

@arcanefoam any movement here?

arcanefoam · 2018-11-19T09:09:07Z

@c4urself sorry for the radio silence, had been working on unrelated projects. I agree with most of the comments from @ekohl, I will reply to them and commit some changes during this week

florisla · 2020-01-13T10:58:57Z

This branch needs a rebase.

arcanefoam added 8 commits February 3, 2018 10:11

add support for comparing versions

f024dd4

Fixes version comparision

3a30f4e

Test bumped part is in parse parts

13b145d

Fixes ge and le comparisions

a79f1db

Add test for parts comparision

9ee9ec7

Merge branch 'master' of https://github.com/c4urself/bump2version

8aa406b

Fixes expected logs in tests for new part check testing

91b213d

Cleanup gitignore

a135eb2

ekohl requested changes Jun 2, 2018

View reviewed changes

Fix request from pull request c4urself#20

23640ac

ekohl reviewed Aug 1, 2018

View reviewed changes

c4urself added the needs-work label Nov 18, 2018

Reply to comments

fc768bd

florisla added the feature label Jan 22, 2021

Add support for version compare and for checking for version part argument #20

Are you sure you want to change the base?

Add support for version compare and for checking for version part argument #20

Uh oh!

Conversation

arcanefoam commented May 10, 2018

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arcanefoam Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arcanefoam Jul 13, 2018 • edited by florisla Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arcanefoam commented Jul 13, 2018

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

florisla Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c4urself commented Nov 18, 2018

Uh oh!

arcanefoam commented Nov 19, 2018

Uh oh!

florisla commented Jan 13, 2020

Uh oh!

Reviewers

arcanefoam Jun 4, 2018 •

edited

Loading

arcanefoam Jul 13, 2018 •

edited by florisla

Loading

florisla Dec 7, 2018 •

edited

Loading