[Asterisk-code-review] version parser: Add a module for parsing Asterisk versions (repotools[master])

[Samuel Galarneau]

Samuel Galarneau has posted comments on this change.

Change subject: version_parser: Add a module for parsing Asterisk versions
......................................................................


Patch Set 2: Code-Review-1

(3 comments)

https://gerrit.asterisk.org/#/c/379/2/version_parser.py
File version_parser.py:

Line 124: previous = AsteriskVersion(self.major, self.minor,
        :             patch=self.patch, modifiers=list(self.modifiers),
        :             prefix=self.prefix, leading_one=self.leading_one)
        :         for i, modifier in enumerate(reversed(previous.modifiers)):
        :             prefix, num = modifier
        :             if num != 1:
        :                 num -= 1
        :                 mod_len = len(previous.modifiers)
        :                 previous.modifiers[mod_len - 1 - i] = (prefix, num)
        :                 previous.modifiers = previous.modifiers[:mod_len - i]
        :                 return previous
        :         # If all of the modifiers were the first of their kind,
        :         # then the previous version took place before any modifiers.
        :         previous.modifiers = []
        :         if previous.patch > 0:
        :             previous.patch -= 1
        :             return previous
        :         previous.patch = 0
        :         if previous.minor > 0:
        :             previous.minor -= 1
        :             return previous
        :         previous.major -= 1
        :         return previous
Whitespace would help readability here.


Line 150: ver = ''
        :         if self.prefix:
        :             ver = '{0}/'.format(self.prefix)
        :         if self.leading_one:
        :             ver = '{0}1.'.format(ver)
        :         ver = '{0}{1}.{2}'.format(ver, self.major, self.minor)
        :         if self.patch > -1:
        :             ver = '{0}.{1}'.format(ver, self.patch)
        :         for mod, num in self.modifiers:
        :             ver = '{0}-{1}{2}'.format(ver, mod, num)
        :         return ver
Whitespace would help readability here.


Line 169: self.assertTrue(ver.leading_one)
        :         self.assertEqual(ver.major, 8)
        :         self.assertEqual(ver.minor, 23)
        :         self.assertEqual(ver.patch, 0)
What about testing that prefix is still None and that modifiers are not present? It may help ensuring regressions are not introduced in the future.


-- 
To view, visit https://gerrit.asterisk.org/379
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I73661d43819388bda383ef4ba1c6ecdeee761fa9
Gerrit-PatchSet: 2
Gerrit-Project: repotools
Gerrit-Branch: master
Gerrit-Owner: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Samuel Galarneau <sgalarneau at digium.com>
Gerrit-HasComments: Yes