Merge lp:~camptocamp/lp-community-utils/nag-user-specific-info into lp:lp-community-utils
- nag-user-specific-info
- Merge into openerp-reviewers-nag
Status: | Merged |
---|---|
Merged at revision: | 33 |
Proposed branch: | lp:~camptocamp/lp-community-utils/nag-user-specific-info |
Merge into: | lp:lp-community-utils |
Diff against target: |
195 lines (+78/-13) 1 file modified
openerp-nag (+78/-13) |
To merge this branch: | bzr merge lp:~camptocamp/lp-community-utils/nag-user-specific-info |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Sylvain LE GAL (GRAP) (community) | code review, no test | Approve | |
Stefan Rijnhart (Opener) | test | Approve | |
Sandy Carter (http://www.savoirfairelinux.com) | grammar | Needs Fixing | |
Guewen Baconnier @ Camptocamp | code review, test | Approve | |
Alexandre Fayolle - camptocamp | code review, test | Approve | |
Review via email: mp+209665@code.launchpad.net |
Commit message
Description of the change
Option --me or --triage to show what I'm supposed to do with that bunch of MPs
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote : | # |
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote : | # |
You can still use --me instead of user_categ.
But I agree user_categ isn't very good. Triage is better.
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote : | # |
Changes made thanks for the review
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote : | # |
There are some inconsistencies in the sentences:
Sometimes the sentences refer to the MPs with a plural form and sometimes with a singular, I prefer the plural form.
Example:
I didn't reviewed those ones
vs
Nothing to do I've already reviewed it
For REVIEWED_UPDATED, I propose 'Check again, updated since the last "Need Fixing" vote', emphasizing on the *again* because the reviewer already reviewed them. I think it should apply for the "Need Information" vote too. In that case it could be 'Check again, updated since the last asking' (same text for both statuses).
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote : | # |
Oh, maybe we could remove the whole sentence " A committer should consider to merge the proposal" and "Someone should review the merge request" when the triage is used, because it's seems very redundant when they are already grouped.
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote : | # |
looks good, works fine. I'm almost +1 for merging : the --user_categ option is not a very good choice imo.
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote : | # |
@Alex oops mispelled the push branche. I pushed it again
- 29. By Yannick Vaucher @ Camptocamp
-
change phrases for more consistancy
- 30. By Yannick Vaucher @ Camptocamp
-
remove sentences when triage is enabled
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote : | # |
Ready for review here
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote : | # |
Yannick,
I think that the "Needs Information" statuses should appear in the REVIEWED_UPDATED section alongside the "Needs Fixing" proposals. Then, the sentence could be "Check again, updated since the last asking" to leave some ambiguity between the 2 statuses.
The change would be on line 115
user_state = REVIEWED_UPDATED if v.date_created < last_commit and v.comment and v.comment.vote in ('Need Fixing', 'Needs Information') else REVIEWED_PENDING
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote : | # |
LGTM
- 31. By Yannick Vaucher @ Camptocamp
-
Add needs information to updated status
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote : | # |
Thanks!
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote : | # |
l.61, l.163 Can we avoid committing commented out code and blank lines.
- 32. By Yannick Vaucher @ Camptocamp
-
remove added space line and clean comented code
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote : | # |
Thanks for the review Sandy
Fixed
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote : | # |
Looks, good, just a few pep8 issues:
openerp-nag:72:9: E126 continuation line over-indented for hanging indent
MY_READY: "My MPs ready to land",
^
openerp-nag:73:9: E126 continuation line over-indented for hanging indent
MY_PENDING: "My MPs needing some love",
^
openerp-nag:74:9: E126 continuation line over-indented for hanging indent
^
openerp-nag:75:9: E126 continuation line over-indented for hanging indent
^
openerp-nag:76:9: E126 continuation line over-indented for hanging indent
^
openerp-nag:77:9: E126 continuation line over-indented for hanging indent
^
openerp-nag:80:1: E302 expected 2 blank lines, found 1
def show_lp_
^
openerp-nag:390:5: E303 too many blank lines (2)
if args.triage:
^
openerp-nag:415:5: E303 too many blank lines (2)
print('Votes legend:')
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote : | # |
A few English fixes needed too. I am not an expert, but I am pretty sure the comma should either be a period or a semicolon, or even a colon in some cases.
24 + MY_READY: "My MPs ready to land",
25 + MY_PENDING: "My MPs needing some love",
26 + REVIEWED_READY: "Ready to land, please Merge us!",
"Ready to land; please Merge us!"
27 + REVIEWED_UPDATED: "To check, updated since last need fixing vote",
"To check; updated since last needs fixing vote"
28 + REVIEWED_PENDING: "Nothing to do I've already reviewed those",
"Nothing to do; I've already reviewed those"
29 + NOT_REVIEWED: "I didn't reviewed those ones",
"I haven't reviewed these ones"
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote : | # |
28 + REVIEWED_PENDING: "Nothing to do I've already reviewed those",
"Nothing to do; I've already reviewed these"
- 33. By Yannick Vaucher @ Camptocamp
-
[PEP8] of user specific nag addition
- 34. By Yannick Vaucher @ Camptocamp
-
English fixes
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote : | # |
Thanks again Sandy
English messages are improved and pep8 of added code is done
Stefan Rijnhart (Opener) (stefan-opener) wrote : | # |
Cool feature!
Sylvain LE GAL (GRAP) (sylvain-legal) wrote : | # |
good feature. (and good project I just discovered!)
LGTM.
Regards.
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote : | # |
Sandy, is it ok for you now?
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote : | # |
Yannick, you will need to rebase unfortunately, the head diverged.
- 35. By Yannick Vaucher @ Camptocamp
-
Merge from upsteam and resolve conflicts
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote : | # |
I resolved conflicts, ready to be merged
Preview Diff
1 | === modified file 'openerp-nag' |
2 | --- openerp-nag 2014-04-25 11:48:45 +0000 |
3 | +++ openerp-nag 2014-04-30 10:17:01 +0000 |
4 | @@ -37,6 +37,7 @@ |
5 | period of time |
6 | * Nagging about aging unanswered questions on launchpad answers |
7 | * Nagging about accepted merge proposals that have not been merged yet |
8 | + * Nagging by triaging MP in categories |
9 | """ |
10 | |
11 | from __future__ import unicode_literals, print_function |
12 | @@ -61,10 +62,21 @@ |
13 | # ...with the specified 'action' |
14 | # ...about a particular 'subject' |
15 | Nag = collections.namedtuple( |
16 | - "Nag", "person action subject sort_class sort_priority sort_age project_name votes") |
17 | + "Nag", "person action subject sort_class sort_priority sort_age project_name votes sort_triage") |
18 | |
19 | SORT_CLASS_BUG, SORT_CLASS_MERGE = range(2) |
20 | |
21 | +MY_READY, MY_PENDING, REVIEWED_READY, REVIEWED_UPDATED, REVIEWED_PENDING, NOT_REVIEWED = range(6) |
22 | + |
23 | +USER_CATEGORIES = { |
24 | + MY_READY: "My MPs ready to land", |
25 | + MY_PENDING: "My MPs needing some love", |
26 | + REVIEWED_READY: "Ready to land; please Merge us!", |
27 | + REVIEWED_UPDATED: "To check; updated since last need fixing vote", |
28 | + REVIEWED_PENDING: "Nothing to do; I've already reviewed these", |
29 | + NOT_REVIEWED: "I haven't reviewed these ones", |
30 | +} |
31 | + |
32 | |
33 | def show_lp_object(obj): |
34 | print(obj) |
35 | @@ -165,7 +177,7 @@ |
36 | state, code in cls.__states__.iteritems()) |
37 | |
38 | |
39 | -def gen_project_nags(lp, policy, project_name): |
40 | +def gen_project_nags(lp, policy, project_name, triage=False): |
41 | # TODO: Detect project groups and redirect the nag to all projects |
42 | # underneath that project |
43 | # Access the project that we care about |
44 | @@ -173,11 +185,11 @@ |
45 | project = lp.projects[project_name] |
46 | # Re-yield all the merge proposal nags |
47 | # NOTE: change to yield from in py3k3 |
48 | - for nag in gen_merge_proposal_nags(lp, policy, project): |
49 | + for nag in gen_merge_proposal_nags(lp, policy, project, triage): |
50 | yield nag |
51 | |
52 | |
53 | -def gen_merge_proposal_nags(lp, policy, project): |
54 | +def gen_merge_proposal_nags(lp, policy, project, triage=False): |
55 | # XXX: cannnot use utcnow as launchpad returns tz-aware objects here |
56 | now = datetime.datetime.now(tz=UTC) |
57 | logging.debug("Looking for merge proposals in project %r", project) |
58 | @@ -208,16 +220,29 @@ |
59 | sort_class=SORT_CLASS_MERGE, |
60 | sort_priority=None, # TODO: get from max(linked bugs) |
61 | sort_age=review_age.days, |
62 | + sort_triage=None, |
63 | project_name=project.name, |
64 | votes=votes, |
65 | ) |
66 | |
67 | # Nag about approved merges |
68 | age = (now - proposal.date_review_requested).days |
69 | + |
70 | + user_state = None |
71 | + if triage: |
72 | + me = lp.me |
73 | + is_my_mp = proposal.registrant == me |
74 | if (votes.approve == votes.total(for_approval=True) and |
75 | (votes.approve >= policy.approvals_to_bypass or |
76 | votes.approve >= policy.min_approve and |
77 | age >= policy.days_before_merge)): |
78 | + |
79 | + if triage: |
80 | + if is_my_mp: |
81 | + user_state = MY_READY |
82 | + else: |
83 | + user_state = REVIEWED_READY |
84 | + |
85 | yield Nag( |
86 | person='A committer', |
87 | action="consider to merge the proposal", |
88 | @@ -225,6 +250,7 @@ |
89 | sort_class=SORT_CLASS_MERGE, |
90 | sort_priority=None, # TODO: get from max(linked bugs) |
91 | sort_age=review_age.days, |
92 | + sort_triage=user_state, |
93 | project_name=project.name, |
94 | votes=votes, |
95 | ) |
96 | @@ -232,6 +258,18 @@ |
97 | |
98 | # Nag about aging merge requests |
99 | if review_age.days >= policy.max_review_age: |
100 | + |
101 | + if triage: |
102 | + if is_my_mp: |
103 | + user_state = MY_PENDING |
104 | + else: |
105 | + user_state = NOT_REVIEWED |
106 | + for v in proposal.votes: |
107 | + if v.reviewer == me: |
108 | + last_commit = proposal.source_branch.date_last_modified |
109 | + user_state = REVIEWED_UPDATED if v.date_created < last_commit and v.comment and v.comment.vote in ('Need Fixing', 'Needs Information') else REVIEWED_PENDING |
110 | + break |
111 | + |
112 | yield Nag( |
113 | person='Someone', |
114 | action="review the merge request", |
115 | @@ -239,6 +277,7 @@ |
116 | sort_class=SORT_CLASS_MERGE, |
117 | sort_priority=None, # TODO: get from max(linked bugs) |
118 | sort_age=review_age.days, |
119 | + sort_triage=user_state, |
120 | project_name=project.name, |
121 | votes=votes, |
122 | ) |
123 | @@ -298,6 +337,10 @@ |
124 | group.add_argument( |
125 | '--production', action='store_const', const='production', |
126 | dest='service_root', help="Use production launchpad instance") |
127 | + group.add_argument( |
128 | + '--triage', '--me', action='store_const', const='triage', |
129 | + help="Use authenticated user to triage merge proposal in categories" |
130 | + "based on their status") |
131 | parser.set_defaults(anonymous=True, service_root='production') |
132 | args = parser.parse_args() |
133 | # Get the policy |
134 | @@ -305,6 +348,9 @@ |
135 | # Reconfigure logging |
136 | if args.debug: |
137 | logging.basicConfig(level=logging.DEBUG) |
138 | + if args.triage: |
139 | + args.authenticated = True |
140 | + args.anonymous = False |
141 | # Access Lauchpad object as the current (system) user or anonymously |
142 | if args.anonymous: |
143 | lp = Launchpad.login_anonymously(consumer_name, args.service_root) |
144 | @@ -331,23 +377,42 @@ |
145 | progress = ProgressBar(widgets=widgets, maxval=len(project_names)).start() |
146 | for project_name in project_names: |
147 | logging.info("Looking for things to nag about under %s", project_name) |
148 | - nags.extend(gen_project_nags(lp, policy, project_name)) |
149 | + nags.extend(gen_project_nags(lp, policy, project_name, args.triage)) |
150 | if ProgressBar: |
151 | progress.update(progress.currval + 1) |
152 | if ProgressBar: |
153 | progress.finish() |
154 | |
155 | - nags.sort(key=lambda nag: (nag.sort_class, nag.sort_priority, |
156 | - -nag.sort_age)) |
157 | + nags.sort(key=lambda nag: (nag.sort_triage, nag.sort_class, |
158 | + nag.sort_priority, -nag.sort_age)) |
159 | print("=" * 80) |
160 | print("Done thinking, here's the nag list".center(80)) |
161 | print("=" * 80) |
162 | - for index1, nag in enumerate(nags, 1): |
163 | - print("{index1:-2}: [age {age}] (votes {votes}) {person} should {action} {subject} " |
164 | - "on the project {project}".format( |
165 | - index1=index1, age=nag.sort_age, person=nag.person, |
166 | - action=nag.action, subject=nag.subject, project=nag.project_name, |
167 | - votes=nag.votes or '')) |
168 | + |
169 | + # Nag with user categories |
170 | + if args.triage: |
171 | + for sort_triage in range(6): |
172 | + categ_nags = [nag for nag in enumerate(nags, 1) |
173 | + if nag[1].sort_triage == sort_triage] |
174 | + if not categ_nags: |
175 | + continue |
176 | + print("=" * 80) |
177 | + print(USER_CATEGORIES[sort_triage].center(80)) |
178 | + print("MP: %d".center(80) % len(categ_nags)) |
179 | + for index1, nag in categ_nags: |
180 | + print("{index1:-2}: [age {age}] (votes {votes}) {subject} " |
181 | + "{project}".format( |
182 | + index1=index1, age=(nag.sort_age and -nag.sort_age), |
183 | + subject=nag.subject, project=nag.project_name, |
184 | + votes=nag.votes or '')) |
185 | + print() |
186 | + else: |
187 | + for index1, nag in enumerate(nags, 1): |
188 | + print("{index1:-2}: [age {age}] (votes {votes}) {person} should {action} {subject} " |
189 | + "on the project {project}".format( |
190 | + index1=index1, age=nag.sort_age, person=nag.person, |
191 | + action=nag.action, subject=nag.subject, project=nag.project_name, |
192 | + votes=nag.votes or '')) |
193 | |
194 | print('Votes legend:') |
195 | if args.anonymous: |
Excellent!
That will be really really useful.
On the output, I thing a blank line could be nice between the sections.
I'm not so enthusiast on the option's name "--user_categ" though. Can I propose --triage? Better idea?