Skip to main content
Code Review

Return to Revisions

2 of 4
Added f-string note
AJNeufeld
  • 35.3k
  • 5
  • 41
  • 103

First of all, have you looked at LooseVersion and/or StrictVersion from distutils.version? They might already provide you with exactly what you need.


General "PEP-8" Comments:

You should have a blank line after your import, after class Version, and after the def __repr__(self) method. The blank-lines inside of __lt__ and __gt__ functions before the else: clause are inconsistent.

Your class and methods could benefit from """doc strings""".

The class members Major, Minor, Patch and Build should all be lower case. Only class names should begin with capital letters. Since the members should be private to the class, they should also be prefixed with an underscore.


The constructor silently fails if ver_str is not exactly a string, and then the instance becomes useless, as the members which are required in every other method of the class become reference errors. There is no need to check if ver_str is a string; just try to split it, and convert the parts to integers. If you haven’t passed in a string, the split or conversion to integers will cause an exception. (You could add a check that the split produces exactly 4 parts, to reject a version such as "1.2.3.4.5.6".)


printme() is unused and could be deleted. If you don’t wish to delete it, at least name it using snake_case to make it more readable: print_me(). You could write the function as simply print(repr(self)) to avoid duplicating code.


__repr__(self) should actually return a string like Version("1.2.3.4"), not the string "1.2.3.4" to conform to the repr() contract. What you have written would make a great __str__(self) method though, and you could then implement __repr__(self) using the str() method.


Inside def main(...):, you are using argv. This is a different argv, shadowing the one you import from sys using from sys import argv.

You should avoid shadowing the imported name, by using:

import sys
# ....
if __name__ == '__main__':
 main(sys.argv)

There is no need to exit(-1) from main(), since the program will terminate anyway immediately. exit() will terminate the Python interpreter, which can be unexpected, and can cause resources to not be properly closed. A unit test framework will abruptly terminate, preventing recording of the test results, and preventing other unit tests from running. In short, don’t ever use exit(). Ever.


Since you are explicitly writing for Python 3.7.0, you can use f-strings. This means you can replace many of your .format(...) calls with a more compact, readable representation where the substitution location and value are combined. Ie:

 print("{} < {}: {}".format(ver_1, ver_2, ver_1 < ver_2))

could become simply:

 print(f"{ver_1} < {ver_2}: {ver_1 < ver_2}")
AJNeufeld
  • 35.3k
  • 5
  • 41
  • 103
default

AltStyle によって変換されたページ (->オリジナル) /