[Asterisk-code-review] digium commits: Improve reliability of extracting user from ... (repotools[master])

[Mark Michelson]

Mark Michelson has posted comments on this change.

Change subject: digium_commits: Improve reliability of extracting user from messages
......................................................................


Patch Set 1: Code-Review-1

(2 comments)

https://gerrit.asterisk.org/#/c/608/1/digium_commits.py
File digium_commits.py:

Line 57:        realname = None
       :         email = None
       :         license = None
       : 
       :         name = raw_user.strip()
       :         if ':' in name:
       :             name.replace(':', '')
       :         if '<' in name:
       :             realname = name[:name.index('<')]
       :             email = name[(name.index('<') + 1):name.index('>')]
       :         if '(' in name:
       :             if not realname:
       :                 realname = name[:name.index('(')]
       :                 license = name[(name.index('(') + 1):name.index(')')]
       :         if not realname:
       :             realname = name
       :         if len(realname) == 0:
       :             return None
       : 
       :         realname = realname.strip()
       :         user = get_user_by_username(realname)
       :         if email:
       :             user.email = email.strip()
       :         if license:
       :             user.license = license.strip()
Would a regex be a good fit for this? As it is right now, this seems prone to error. For instance, if there is an opening '(' or '<' but no closing one, I'm not sure what would happen. The python documentation for index() says "Return the index in the list of the first item whose value is x. It is an error if there is no such item." I'm not sure whether "It is an error" means that an exception will be thrown if the item is not in the sequence.


Line 163:             if 'uploaded by' in token.lower():
        :                 name = token[token.lower().index('uploaded by') + 11:]
        :             if 'submitted by' in token.lower():
        :                 name = token[token.lower().index('submitted by') + 12:]
If you're normalizing these sorts of things, should "uploaded by" and "submitted by" also accept a hyphen as the separator? Similarly, should they take into account an optional colon at the end of the string?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id85d719aa2cc59015f1b49539563ddbb7b2b1928
Gerrit-PatchSet: 1
Gerrit-Project: repotools
Gerrit-Branch: master
Gerrit-Owner: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes