Merge lp:~brian-murray/cupstream2distro/bug-1460861 into lp:cupstream2distro

[Brian Murray]

[Robert Bruce Park (robru)]

1. Please drop ordereddict and just do the sorting when reading from the dict rather than sitting the input to the dict.

2. I'm not crazy about the implementation, i feel like it could be cleaner with something like this:

msgs = defaultdict(list)
for author, msg in authors.items():
    msgs[msg].append[author]
authors = {', '.join(ppl): msg for msg, ppl in msgs}

But also look into the called function, have it generate the dict correctly in the first place, rather than converting it and converting it back.

[Robert Bruce Park (robru)]

s/sitting/sorting/, sorry, on phone

[Robert Bruce Park (robru)]

Sorry, my comment about "called function" is wrong, this is the correct place to be working on, I'd just like to see a dict where the keys are messages and the values are lists of authors, rather than trying to munge the author list as a string and then deleting the old key every time you append.

[Brian Murray (brian-murray)]

messages can't be a key because those they are lists e.g.:

Traceback (most recent call last):
  File "/home/bdmurray/source-trees/cupstream2distro/trunk/tests/unit/test_branchhandling.py", line 403, in test_collect_author_with_launchpad_bot_commits
    self.assertEquals(self.branch.get_branch_commits(set()), ({
  File "/home/bdmurray/source-trees/cupstream2distro/trunk/cupstream2distro/branchhandling.py", line 307, in get_branch_commits
    msgs[msg].append(author)
TypeError: unhashable type: 'list'

Do you have any other ideas?

[Robert Bruce Park (robru)]

Ah, sorry. Make the list elements into the dict keys, so then something like:

by_msg = defaultdict(list)
for author, msgs in authors.items():
    for msg in msgs:
        by_msg[msg].append[author]
authors = {', '.join(sorted(ppl)): msg for msg, ppl in msgs}

The idea is that each individual commit message should only show up once but have a list of authors attached to it, rather than having authors show up once and have a list of messages attached to that.

[lp:~brian-murray/cupstream2distro/bug-1460861]

1356. By Brian Murray on 2016-02-05

change how the author's dict is created per robru

1357. By Brian Murray on 2016-02-08

address reviewer feedback, stop using an OrderedDict

[Robert Bruce Park (robru)]

one comment inline.

[Brian Murray (brian-murray)]

Its not clear to me what you are looking for now. Could you elaborate?

On Mon, Feb 08, 2016 at 07:22:06PM -0000, Robert Bruce Park wrote:
> one comment inline.
>
> Diff comments:
>
> >
> > === modified file 'tests/unit/test_branchhandling.py'
> > --- tests/unit/test_branchhandling.py 2015-12-04 05:58:26 +0000
> > +++ tests/unit/test_branchhandling.py 2016-02-08 19:03:59 +0000
> > @@ -330,22 +328,24 @@
> > def test_collect_author_commits_regular(self):
> > """Collect author commits and bugs for a normal bzr log."""
> > self.mock_diff_since_newest_tag('classiclogdiff')
> > - self.assertEquals(self.branch.get_branch_commits(set()), ({
> > - 'Sebastien Bacher': [
> > - "Use '%s:' string for preview hints, rather than just "
> > - "appending ':'. (LP: #1074038)"
> > - ], 'Brandon Schaefer': [
> > + commits = self.branch.get_branch_commits(set())
> > + commits = sorted(commits.items())
> > + self.assertEquals(commits, ([
> > + ('Andrea Azzarone', [
> > + 'Disable detail view for webapp icons. (LP: #1169340)'
> > + ]), ('Brandon Schaefer, Marco Trevisan (Treviño)', [
>
> Please changing this list of tuples back into a dictionary, will avoid the sorting issues with trying to test against the value of "commits.items()"
>
> > 'EdgeBarrierController: add multiple types of subscriber '
> > 'results, fix edges on autohide (LP: #1064945)'
> > - ], 'Marco Trevisan (Treviño)': [
> > - 'Simulating direct commit to trunk',
> > - 'EdgeBarrierController: add multiple types of subscriber '
> > - 'results, fix edges on autohide (LP: #1064945)',
> > + ]), ('Marco Trevisan (Treviño)', [
> > 'IconRenderer: preprocess an icon if its emblem has been '
> > 'shown/hidden (LP: #1171476, #1171663)',
> > + 'Simulating direct commit to trunk',
> > "UnityWindow: don't draw the panel shadow above a "
> > - "fullscreen window. (LP: #1171934)"
> > - ], "Łukasz 'sil2100' Zemczak": [
> > + "fullscreen window. (LP: #1171934)",
> > + ]), ('Sebastien Bacher', [
> > + "Use '%s:' string for preview hints, rather than just "
> > + "appending ':'. (LP: #1074038)",
> > + ]), ("Łukasz 'sil2100' Zemczak", [
> > 'Now that we\'re using the new HUD, there have been some '
> > 'changes that typically cause test_hud tests to fail. '
> > 'Fix the tests to fit the new model. The first one is '
>
>
> --
> https://code.launchpad.net/~brian-murray/cupstream2distro/bug-1460861/+merge/285115
> You are the owner of lp:~brian-murray/cupstream2distro/bug-1460861.
>
--
Brian Murray

[Robert Bruce Park (robru)]

Look at your diff in the test suite. There are two changes happening:

1) you made the desired change of 'Author 1, Author 2' in the author names.

2) You changed a perfectly good dictionary into this bizarre list of tuples and it's significantly more difficult to read. so please change this:

+ commits = self.branch.get_branch_commits(set())
+ commits = sorted(commits.items())
+ self.assertEquals(commits, ([
+ ('Andrea Azzarone', [
+ 'Disable detail view for webapp icons. (LP: #1169340)'

Back to this:

- self.assertEquals(self.branch.get_branch_commits(set()), ({
- 'Sebastien Bacher': [
- "Use '%s:' string for preview hints, rather than just "
- "appending ':'. (LP: #1074038)"

The diff will guide you. Just eliminate all the changes that aren't string changes.

[Robert Bruce Park (robru)]

one more comment inline

[Brian Murray (brian-murray)]

On Mon, Feb 08, 2016 at 09:40:28PM -0000, Robert Bruce Park wrote:
> Look at your diff in the test suite. There are two changes happening:
>
> 1) you made the desired change of 'Author 1, Author 2' in the author names.
>
> 2) You changed a perfectly good dictionary into this bizarre list of tuples and it's significantly more difficult to read. so please change this:
>
> + commits = self.branch.get_branch_commits(set())
> + commits = sorted(commits.items())
> + self.assertEquals(commits, ([
> + ('Andrea Azzarone', [
> + 'Disable detail view for webapp icons. (LP: #1169340)'
>
> Back to this:
>
> - self.assertEquals(self.branch.get_branch_commits(set()), ({
> - 'Sebastien Bacher': [
> - "Use '%s:' string for preview hints, rather than just "
> - "appending ':'. (LP: #1074038)"
>
> The diff will guide you. Just eliminate all the changes that aren't string changes.

The tests were behaving erratically unless the data was sorted in this
way.

--
Brian Murray

[Robert Bruce Park (robru)]

> The tests were behaving erratically unless the data was sorted in this
> way.

That's because you were trying to compare a dict to an ordereddict. I'm asking you to compare an unordered dict to an unordered dict, the order will not matter.

[lp:~brian-murray/cupstream2distro/bug-1460861]

1358. By Brian Murray on 2016-02-09

restore tests to their original version more or less

[Brian Murray (brian-murray)]

I'm pretty sure I had removed the OrderedDict and was still having issues with things be out of order, perhaps the failure was really with the msgs for a specific author. Regardless, I've restored the tests now.

[Robert Bruce Park (robru)]

Great, thanks, diff looks way nicer, will merge soon.

review: Approve

=== modified file 'cupstream2distro/branchhandling.py'
--- cupstream2distro/branchhandling.py	2016-02-08 19:46:41 +0000
+++ cupstream2distro/branchhandling.py	2016-02-09 20:31:59 +0000
@@ -273,6 +273,7 @@
         :returns: {'Author': ['Commit 1', 'Commit 2']}
         """
         authors = defaultdict(list)
+        by_msgs = defaultdict(list)
         data = defaultdict(str)
         for data in parse_bzr_log(self.diff_since_newest_branch_tag()):
             msg = data['message:'].rsplit('Approved by', 1)[0].strip()
@@ -287,6 +288,14 @@
                     author = author.split(' <')[0].strip()
                     if not ignored(author):
                         authors[author].append(msg + format_bugs(bugs))
+        for author, msgs in authors.items():
+            for msg in msgs:
+                by_msgs[msg].append(author)
+        authors.clear()
+        for msg, ppl in by_msgs.items():
+            authors[', '.join(sorted(ppl))].append(msg)
+        for author, msgs in authors.items():
+            authors[author] = sorted(msgs)
         return authors
 
     def missing_branch_commits(self, branch, target):

=== modified file 'tests/unit/test_branchhandling.py'
--- tests/unit/test_branchhandling.py	2016-02-08 19:46:41 +0000
+++ tests/unit/test_branchhandling.py	2016-02-09 20:31:59 +0000
@@ -310,10 +310,8 @@
         """Collect author commits and bugs when the msg starts with '-'."""
         self.mock_diff_since_newest_tag('withdashlogdiff')
         self.assertEquals(self.branch.get_branch_commits(set()), ({
-            'Diego Sarmentero': [
-                'Update download manager API (BUG: #1224538). (LP: #1224538)',
-            ], 'Manuel de la Pena': [
-                'Update download manager API (BUG: #1224538). (LP: #1224538)',
+            'Diego Sarmentero, Manuel de la Pena': [
+                'Update download manager API (BUG: #1224538). (LP: #1224538)'
             ]}))
 
     def test_collect_author_commits_starting_with_star(self):
@@ -334,15 +332,13 @@
             'Sebastien Bacher': [
                 "Use '%s:' string for preview hints, rather than just "
                 "appending ':'. (LP: #1074038)"
-            ], 'Brandon Schaefer': [
+            ], 'Brandon Schaefer, Marco Trevisan (Treviño)': [
                 'EdgeBarrierController: add multiple types of subscriber '
                 'results, fix edges on autohide (LP: #1064945)'
             ], 'Marco Trevisan (Treviño)': [
-                'Simulating direct commit to trunk',
-                'EdgeBarrierController: add multiple types of subscriber '
-                'results, fix edges on autohide (LP: #1064945)',
                 'IconRenderer: preprocess an icon if its emblem has been '
                 'shown/hidden (LP: #1171476, #1171663)',
+                'Simulating direct commit to trunk',
                 "UnityWindow: don't draw the panel shadow above a "
                 "fullscreen window. (LP: #1171934)"
             ], "Łukasz 'sil2100' Zemczak": [
@@ -367,9 +363,9 @@
                     "Use '%s:' string for preview hints, rather than just "
                     "appending ':'. (LP: #1074038)"
                 ], 'Marco Trevisan (Treviño)': [
-                    'Simulating direct commit to trunk',
                     'IconRenderer: preprocess an icon if its emblem has been '
                     'shown/hidden (LP: #1171476, #1171663)',
+                    'Simulating direct commit to trunk',
                     "UnityWindow: don't draw the panel shadow above a "
                     'fullscreen window. (LP: #1171934)'
                 ], "Łukasz 'sil2100' Zemczak": [
@@ -407,15 +403,13 @@
             'Sebastien Bacher': [
                 "Use '%s:' string for preview hints, rather than just "
                 "appending ':'. (LP: #1074038)"
-            ], 'Brandon Schaefer': [
+            ], 'Brandon Schaefer, Marco Trevisan (Treviño)': [
                 'EdgeBarrierController: add multiple types of subscriber '
                 'results, fix edges on autohide (LP: #1064945)'
             ], 'Marco Trevisan (Treviño)': [
-                'Simulating direct commit to trunk',
-                'EdgeBarrierController: add multiple types of subscriber '
-                'results, fix edges on autohide (LP: #1064945)',
                 'IconRenderer: preprocess an icon if its emblem has been '
                 'shown/hidden (LP: #1171476, #1171663)',
+                'Simulating direct commit to trunk',
                 "UnityWindow: don't draw the panel shadow above a "
                 'fullscreen window. (LP: #1171934)'
             ], "Łukasz 'sil2100' Zemczak": [