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

[Mark Michelson]

Mark Michelson has posted comments on this change.

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


Patch Set 3: Code-Review-1

(7 comments)

I don't particularly like the method currently being used for HTML generation. Writing raw HTML like this leaves the generated HTML open to injection-style vulnerabilities (I don't know where this HTML is served from, tbh). Using something like https://pypi.python.org/pypi/html/ seems like a decent alternative since it's actually easier to write than the current code and would be easier to maintain/alter.

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.maxint - x[0], x[1]))

Just a suggestion.

Also, I may be misinterpreting, but it appears the idea here by using sys.maxint is to sort by highest count to lowest count. Would setting reverse=True on the sort(ed) call get the proper behavior? Or would that end up sorting the second component of the tuple in an undesirable way?


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.


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


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 commit? "Merge" isn't that uncommon of a word, so commit messages like "Merge the foo and bar objects into one" would trip this condition.


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" ?


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 thing:

issue_dict.setdefault(issuetype, []).append(issue)

https://docs.python.org/2/library/stdtypes.html#dict.setdefault


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


-- 
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-HasComments: Yes