[Asterisk-code-review] release summary: Refactor module to support Git (repotools[master])

[Matt Jordan]

Matt Jordan has posted comments on this change.

Change subject: release_summary: Refactor module to support Git
......................................................................


Patch Set 3:

(7 comments)

https://gerrit.asterisk.org/#/c/179/3/release_summary.py
File release_summary.py:

Line 54:         for key in self.reporter_dic.iterkeys():
       :             self.reporter_sorted.append(self.reporter_dic[key])
       :         self.reporter_sorted.sort(key=lambda x:
       :                                   (sys.maxint - x[0], x[1]))
> self.reporter_sorted = sorted(reporter_dic.itervalues(), key=lambda x: (sys
Yeah, that's a lot better.


Line 110:     RELEASE_TYPE_FEATURE = 1
        :     RELEASE_TYPE_BUGFIX = 2
        :     RELEASE_TYPE_SECURITY = 3
        : 
        :     @classmethod
        :     def get_release_type(cls, release_type):
        :         """Get the release type from a string representation
        : 
        :         Keyword Arguments:
        :         release_type - The type of release
        : 
        :         Returns:
        :         An integer value that represents the type of release
        :         """
        :         if release_type == "bugfix":
        :             return ReleaseSummaryOptions.RELEASE_TYPE_BUGFIX
        :         elif release_type == "feature":
        :             return ReleaseSummaryOptions.RELEASE_TYPE_FEATURE
        :         elif release_type == "security":
        :             return ReleaseSummaryOptions.RELEASE_TYPE_SECURITY
        :         else:
        :             raise Exception("Unknown release type: {0}".format(
        :                 release_type))
> Seems like a good use case for a dictionary.
Fixed.


Line 156:         "package. Since this is new major release, users are encouraged to " \
> s/this is new/this is a new/
Fixed.


Line 261:             if log_message.get_commit_summary().startswith("Merge"):
        :                 # Ignore merge commits
        :                 continue
> Hm, is this really the best way to determine if the commit is a merge commi
As it turns out, a merge commit always has more than one parent. So the following should work:

if log_message.raw and log_message.raw.parents > 1:


Line 265:             # Reporters is the aggregate of what is reportee, and
        :             # what JIRA reports back to us
> Not sure what "what is reportee" is supposed to be. Maybe "what is reported
That's a bad comment even if the spelling was correct. It should be more like:

"Reporters is the aggregate of those reporters in the commit message and what JIRA records as the issue reporter"


Line 313:                     if issuetype not in issue_dict:
        :                         issue_dict[issuetype] = []
        :                     issue_dict[issuetype].append(issue)
> Python dicts have a setdefault() method that you can use for this sort of t
Nifty.


Line 414:                 if component.name not in components:
        :                     components[component.name] = []
        :                 components[component.name].append(issue)
> Another place where setdefault() would work.
Fixed.


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

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