Merge lp:~brian-murray/cupstream2distro/bug-1460861 into lp:cupstream2distro
- bug-1460861
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Robert Bruce Park |
Approved revision: | 1358 |
Merged at revision: | 1366 |
Proposed branch: | lp:~brian-murray/cupstream2distro/bug-1460861 |
Merge into: | lp:cupstream2distro |
Diff against target: |
90 lines (+16/-13) 2 files modified
cupstream2distro/branchhandling.py (+9/-0) tests/unit/test_branchhandling.py (+7/-13) |
To merge this branch: | bzr merge lp:~brian-murray/cupstream2distro/bug-1460861 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Robert Bruce Park (community) | Approve | ||
Review via email: mp+285115@code.launchpad.net |
Commit message
Description of the change
Robert Bruce Park (robru) wrote : | # |
Robert Bruce Park (robru) wrote : | # |
s/sitting/sorting/, sorry, on phone
Robert Bruce Park (robru) wrote : | # |
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) wrote : | # |
messages can't be a key because those they are lists e.g.:
Traceback (most recent call last):
File "/home/
self.
File "/home/
msgs[
TypeError: unhashable type: 'list'
Do you have any other ideas?
Robert Bruce Park (robru) wrote : | # |
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:
authors = {', '.join(
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.
- 1356. By Brian Murray
-
change how the author's dict is created per robru
- 1357. By Brian Murray
-
address reviewer feedback, stop using an OrderedDict
Robert Bruce Park (robru) wrote : | # |
one comment inline.
Brian Murray (brian-murray) wrote : | # |
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/
> > --- tests/unit/
> > +++ tests/unit/
> > @@ -330,22 +328,24 @@
> > def test_collect_
> > """Collect author commits and bugs for a normal bzr log."""
> > self.mock_
> > - self.assertEqua
> > - 'Sebastien Bacher': [
> > - "Use '%s:' string for preview hints, rather than just "
> > - "appending ':'. (LP: #1074038)"
> > - ], 'Brandon Schaefer': [
> > + commits = self.branch.
> > + commits = sorted(
> > + self.assertEqua
> > + ('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()"
>
> > 'EdgeBarrierCon
> > 'results, fix edges on autohide (LP: #1064945)'
> > - ], 'Marco Trevisan (Treviño)': [
> > - 'Simulating direct commit to trunk',
> > - 'EdgeBarrierCon
> > - '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:/
> You are the owner of lp:~brian-murray/cupstream2distro/bug-1460861.
>
--
Brian Murray
Robert Bruce Park (robru) 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.
+ commits = sorted(
+ self.assertEqua
+ ('Andrea Azzarone', [
+ 'Disable detail view for webapp icons. (LP: #1169340)'
Back to this:
- self.assertEqua
- '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) wrote : | # |
one more comment inline
Brian Murray (brian-murray) wrote : | # |
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.
> + commits = sorted(
> + self.assertEqua
> + ('Andrea Azzarone', [
> + 'Disable detail view for webapp icons. (LP: #1169340)'
>
> Back to this:
>
> - self.assertEqua
> - '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) wrote : | # |
> 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.
- 1358. By Brian Murray
-
restore tests to their original version more or less
Brian Murray (brian-murray) wrote : | # |
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) wrote : | # |
Great, thanks, diff looks way nicer, will merge soon.
Preview Diff
1 | === modified file 'cupstream2distro/branchhandling.py' | |||
2 | --- cupstream2distro/branchhandling.py 2016-02-08 19:46:41 +0000 | |||
3 | +++ cupstream2distro/branchhandling.py 2016-02-09 20:31:59 +0000 | |||
4 | @@ -273,6 +273,7 @@ | |||
5 | 273 | :returns: {'Author': ['Commit 1', 'Commit 2']} | 273 | :returns: {'Author': ['Commit 1', 'Commit 2']} |
6 | 274 | """ | 274 | """ |
7 | 275 | authors = defaultdict(list) | 275 | authors = defaultdict(list) |
8 | 276 | by_msgs = defaultdict(list) | ||
9 | 276 | data = defaultdict(str) | 277 | data = defaultdict(str) |
10 | 277 | for data in parse_bzr_log(self.diff_since_newest_branch_tag()): | 278 | for data in parse_bzr_log(self.diff_since_newest_branch_tag()): |
11 | 278 | msg = data['message:'].rsplit('Approved by', 1)[0].strip() | 279 | msg = data['message:'].rsplit('Approved by', 1)[0].strip() |
12 | @@ -287,6 +288,14 @@ | |||
13 | 287 | author = author.split(' <')[0].strip() | 288 | author = author.split(' <')[0].strip() |
14 | 288 | if not ignored(author): | 289 | if not ignored(author): |
15 | 289 | authors[author].append(msg + format_bugs(bugs)) | 290 | authors[author].append(msg + format_bugs(bugs)) |
16 | 291 | for author, msgs in authors.items(): | ||
17 | 292 | for msg in msgs: | ||
18 | 293 | by_msgs[msg].append(author) | ||
19 | 294 | authors.clear() | ||
20 | 295 | for msg, ppl in by_msgs.items(): | ||
21 | 296 | authors[', '.join(sorted(ppl))].append(msg) | ||
22 | 297 | for author, msgs in authors.items(): | ||
23 | 298 | authors[author] = sorted(msgs) | ||
24 | 290 | return authors | 299 | return authors |
25 | 291 | 300 | ||
26 | 292 | def missing_branch_commits(self, branch, target): | 301 | def missing_branch_commits(self, branch, target): |
27 | 293 | 302 | ||
28 | === modified file 'tests/unit/test_branchhandling.py' | |||
29 | --- tests/unit/test_branchhandling.py 2016-02-08 19:46:41 +0000 | |||
30 | +++ tests/unit/test_branchhandling.py 2016-02-09 20:31:59 +0000 | |||
31 | @@ -310,10 +310,8 @@ | |||
32 | 310 | """Collect author commits and bugs when the msg starts with '-'.""" | 310 | """Collect author commits and bugs when the msg starts with '-'.""" |
33 | 311 | self.mock_diff_since_newest_tag('withdashlogdiff') | 311 | self.mock_diff_since_newest_tag('withdashlogdiff') |
34 | 312 | self.assertEquals(self.branch.get_branch_commits(set()), ({ | 312 | self.assertEquals(self.branch.get_branch_commits(set()), ({ |
39 | 313 | 'Diego Sarmentero': [ | 313 | 'Diego Sarmentero, Manuel de la Pena': [ |
40 | 314 | 'Update download manager API (BUG: #1224538). (LP: #1224538)', | 314 | 'Update download manager API (BUG: #1224538). (LP: #1224538)' |
37 | 315 | ], 'Manuel de la Pena': [ | ||
38 | 316 | 'Update download manager API (BUG: #1224538). (LP: #1224538)', | ||
41 | 317 | ]})) | 315 | ]})) |
42 | 318 | 316 | ||
43 | 319 | def test_collect_author_commits_starting_with_star(self): | 317 | def test_collect_author_commits_starting_with_star(self): |
44 | @@ -334,15 +332,13 @@ | |||
45 | 334 | 'Sebastien Bacher': [ | 332 | 'Sebastien Bacher': [ |
46 | 335 | "Use '%s:' string for preview hints, rather than just " | 333 | "Use '%s:' string for preview hints, rather than just " |
47 | 336 | "appending ':'. (LP: #1074038)" | 334 | "appending ':'. (LP: #1074038)" |
49 | 337 | ], 'Brandon Schaefer': [ | 335 | ], 'Brandon Schaefer, Marco Trevisan (Treviño)': [ |
50 | 338 | 'EdgeBarrierController: add multiple types of subscriber ' | 336 | 'EdgeBarrierController: add multiple types of subscriber ' |
51 | 339 | 'results, fix edges on autohide (LP: #1064945)' | 337 | 'results, fix edges on autohide (LP: #1064945)' |
52 | 340 | ], 'Marco Trevisan (Treviño)': [ | 338 | ], 'Marco Trevisan (Treviño)': [ |
53 | 341 | 'Simulating direct commit to trunk', | ||
54 | 342 | 'EdgeBarrierController: add multiple types of subscriber ' | ||
55 | 343 | 'results, fix edges on autohide (LP: #1064945)', | ||
56 | 344 | 'IconRenderer: preprocess an icon if its emblem has been ' | 339 | 'IconRenderer: preprocess an icon if its emblem has been ' |
57 | 345 | 'shown/hidden (LP: #1171476, #1171663)', | 340 | 'shown/hidden (LP: #1171476, #1171663)', |
58 | 341 | 'Simulating direct commit to trunk', | ||
59 | 346 | "UnityWindow: don't draw the panel shadow above a " | 342 | "UnityWindow: don't draw the panel shadow above a " |
60 | 347 | "fullscreen window. (LP: #1171934)" | 343 | "fullscreen window. (LP: #1171934)" |
61 | 348 | ], "Łukasz 'sil2100' Zemczak": [ | 344 | ], "Łukasz 'sil2100' Zemczak": [ |
62 | @@ -367,9 +363,9 @@ | |||
63 | 367 | "Use '%s:' string for preview hints, rather than just " | 363 | "Use '%s:' string for preview hints, rather than just " |
64 | 368 | "appending ':'. (LP: #1074038)" | 364 | "appending ':'. (LP: #1074038)" |
65 | 369 | ], 'Marco Trevisan (Treviño)': [ | 365 | ], 'Marco Trevisan (Treviño)': [ |
66 | 370 | 'Simulating direct commit to trunk', | ||
67 | 371 | 'IconRenderer: preprocess an icon if its emblem has been ' | 366 | 'IconRenderer: preprocess an icon if its emblem has been ' |
68 | 372 | 'shown/hidden (LP: #1171476, #1171663)', | 367 | 'shown/hidden (LP: #1171476, #1171663)', |
69 | 368 | 'Simulating direct commit to trunk', | ||
70 | 373 | "UnityWindow: don't draw the panel shadow above a " | 369 | "UnityWindow: don't draw the panel shadow above a " |
71 | 374 | 'fullscreen window. (LP: #1171934)' | 370 | 'fullscreen window. (LP: #1171934)' |
72 | 375 | ], "Łukasz 'sil2100' Zemczak": [ | 371 | ], "Łukasz 'sil2100' Zemczak": [ |
73 | @@ -407,15 +403,13 @@ | |||
74 | 407 | 'Sebastien Bacher': [ | 403 | 'Sebastien Bacher': [ |
75 | 408 | "Use '%s:' string for preview hints, rather than just " | 404 | "Use '%s:' string for preview hints, rather than just " |
76 | 409 | "appending ':'. (LP: #1074038)" | 405 | "appending ':'. (LP: #1074038)" |
78 | 410 | ], 'Brandon Schaefer': [ | 406 | ], 'Brandon Schaefer, Marco Trevisan (Treviño)': [ |
79 | 411 | 'EdgeBarrierController: add multiple types of subscriber ' | 407 | 'EdgeBarrierController: add multiple types of subscriber ' |
80 | 412 | 'results, fix edges on autohide (LP: #1064945)' | 408 | 'results, fix edges on autohide (LP: #1064945)' |
81 | 413 | ], 'Marco Trevisan (Treviño)': [ | 409 | ], 'Marco Trevisan (Treviño)': [ |
82 | 414 | 'Simulating direct commit to trunk', | ||
83 | 415 | 'EdgeBarrierController: add multiple types of subscriber ' | ||
84 | 416 | 'results, fix edges on autohide (LP: #1064945)', | ||
85 | 417 | 'IconRenderer: preprocess an icon if its emblem has been ' | 410 | 'IconRenderer: preprocess an icon if its emblem has been ' |
86 | 418 | 'shown/hidden (LP: #1171476, #1171663)', | 411 | 'shown/hidden (LP: #1171476, #1171663)', |
87 | 412 | 'Simulating direct commit to trunk', | ||
88 | 419 | "UnityWindow: don't draw the panel shadow above a " | 413 | "UnityWindow: don't draw the panel shadow above a " |
89 | 420 | 'fullscreen window. (LP: #1171934)' | 414 | 'fullscreen window. (LP: #1171934)' |
90 | 421 | ], "Łukasz 'sil2100' Zemczak": [ | 415 | ], "Łukasz 'sil2100' Zemczak": [ |
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) msg].append[ author]
for author, msg in authors.items():
msgs[
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.