Issue 29363768: Fixes 4662 - [hgreview] Add an option for suppressing the email after upload (Closed)
|
Created:
Nov. 22, 2016, 3:01 p.m. by Felix Dahlke Modified:
Nov. 28, 2016, 9:09 a.m. Base URL:
https://bitbucket.org/fhd/codingtools Visibility:
Public. |
DescriptionFixes 4662 - [hgreview] Add an option for suppressing the email after upload
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address review feedback #Patch Set 3 : Only print a warning when no reviewers have been specified #
Total comments: 2
Patch Set 4 : Use ui.status instead of print #MessagesTotal messages: 13
Hi Felix, I haven't tested the changes, but it looks good in general. I noticed a few code style issues (see below). The line length stuff is not really a deal breaker, but the quotes should probably be corrected. Cheers, Vasily https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py#newcode45 hgreview.py:45: if (not opts.get("no_mail") and According to our style guide this string should be in single quotes (we've introduced a rule about this some time ago, see https://adblockplus.org/coding-style#python, third bullet point). https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py#newcode46 hgreview.py:46: (not opts.get('issue') or opts.get('message'))): Flake8 also gives E129 on this line (suggesting to add one more indent to distinguish it from the following line). I'm not sure I could say that the result is more beautiful that way but it is perhaps a tiny bit more readable. I guess I'll leave it up to you and just say that I have a slight preference towards making Flake8 happy unless it wants something ridiculous -- automated checks (which I hope we'll add here too eventually) are worth a bit of constraints. https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py#newcode81 hgreview.py:81: opts['reviewers'] = ui.prompt('Reviewers (comma-separated): ', '') This line is now 82 characters, so it probably would be nice to wrap it. I would agree that PEP8 is a bit too strict about line length, but if we want to change the limit for our style guide, we should probably do so explicitly. Perhaps we could have something like "aspire for <80 but up to 85 is ok" and set the automatic tools to have a hard limit of 85. Anyway, this discussion is not for this review.
https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py#newcode29 hgreview.py:29: ('', 'no_mail', None, 'Don\'t send an email after uploading.', 'NO_MAIL'), This is a flag without any parameter - please drop 'NO_MAIL'. https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py#newcode81 hgreview.py:81: opts['reviewers'] = ui.prompt('Reviewers (comma-separated): ', '') I'd still ask for reviewers here but accept empty input. Not being reminded would be an unexpected side-effect of this option.
New patch set is up, all comments addressed. https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py#newcode29 hgreview.py:29: ('', 'no_mail', None, 'Don\'t send an email after uploading.', 'NO_MAIL'), On 2016/11/22 15:39:35, Wladimir Palant wrote: > This is a flag without any parameter - please drop 'NO_MAIL'. Done. https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py#newcode45 hgreview.py:45: if (not opts.get("no_mail") and On 2016/11/22 15:29:35, Vasily Kuznetsov wrote: > According to our style guide this string should be in single quotes (we've > introduced a rule about this some time ago, see > https://adblockplus.org/coding-style#python, third bullet point). Whoops, something I'll have to get used to :P Done. https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py#newcode46 hgreview.py:46: (not opts.get('issue') or opts.get('message'))): On 2016/11/22 15:29:35, Vasily Kuznetsov wrote: > Flake8 also gives E129 on this line (suggesting to add one more indent to > distinguish it from the following line). I'm not sure I could say that the > result is more beautiful that way but it is perhaps a tiny bit more readable. > > I guess I'll leave it up to you and just say that I have a slight preference > towards making Flake8 happy unless it wants something ridiculous -- automated > checks (which I hope we'll add here too eventually) are worth a bit of > constraints. Forgout about the whole flake8 thing. Running it on this file, there's a bunch of things that need to be fixed (mainly line length). But this particular line I can't fix. If I indent the second line of the condition some more, flake8 complains that it is over-indented. If I indent it less, it complains about it being under-indented... I suggest we figure out how to deal with that warning globally, and then fix it here along with the other issues. https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py#newcode81 hgreview.py:81: opts['reviewers'] = ui.prompt('Reviewers (comma-separated): ', '') On 2016/11/22 15:39:35, Wladimir Palant wrote: > I'd still ask for reviewers here but accept empty input. Not being reminded > would be an unexpected side-effect of this option. I personally prefer to add reviewers via the web interface, but I can live with prompting for reviewers without requiring them. Done. https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py#newcode81 hgreview.py:81: opts['reviewers'] = ui.prompt('Reviewers (comma-separated): ', '') On 2016/11/22 15:29:35, Vasily Kuznetsov wrote: > This line is now 82 characters, so it probably would be nice to wrap it. > > I would agree that PEP8 is a bit too strict about line length, but if we want to > change the limit for our style guide, we should probably do so explicitly. > Perhaps we could have something like "aspire for <80 but up to 85 is ok" and set > the automatic tools to have a hard limit of 85. Anyway, this discussion is not > for this review. I'm actually a pretty big fan of staying within the 80 line limit unless it hurts readability considerably. Here I haven't bothered so much since the surrounding code doesn't, but that's possibly not the best attitude . Would have fixed this, but it was fixed as a side effect of removing the condition I introduced :P
LGTM https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py#newcode46 hgreview.py:46: (not opts.get('issue') or opts.get('message'))): On 2016/11/23 05:46:48, Felix Dahlke wrote: > On 2016/11/22 15:29:35, Vasily Kuznetsov wrote: > > Flake8 also gives E129 on this line (suggesting to add one more indent to > > distinguish it from the following line). I'm not sure I could say that the > > result is more beautiful that way but it is perhaps a tiny bit more readable. > > > > I guess I'll leave it up to you and just say that I have a slight preference > > towards making Flake8 happy unless it wants something ridiculous -- automated > > checks (which I hope we'll add here too eventually) are worth a bit of > > constraints. > > Forgout about the whole flake8 thing. Running it on this file, there's a bunch > of things that need to be fixed (mainly line length). But this particular line I > can't fix. If I indent the second line of the condition some more, flake8 > complains that it is over-indented. If I indent it less, it complains about it > being under-indented... I suggest we figure out how to deal with that warning > globally, and then fix it here along with the other issues. For me it seemed to work with an additional 4-space indent, but no big deal, indeed there are lots of other flake8 warnings around anyway. https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py#newcode81 hgreview.py:81: opts['reviewers'] = ui.prompt('Reviewers (comma-separated): ', '') On 2016/11/23 05:46:48, Felix Dahlke wrote: > On 2016/11/22 15:39:35, Wladimir Palant wrote: > > I'd still ask for reviewers here but accept empty input. Not being reminded > > would be an unexpected side-effect of this option. > > I personally prefer to add reviewers via the web interface, but I can live with > prompting for reviewers without requiring them. Done. I share Felix's preference, probably because I'm basically using the same workflow (upload, check that everything looks right via the web UI, add reviewers, send notifications). I can live with the way it's implemented in the patch set 2 but I'm not sure if this is really helpful.
https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py#newcode81 hgreview.py:81: opts['reviewers'] = ui.prompt('Reviewers (comma-separated): ', '') On 2016/11/23 11:00:28, Vasily Kuznetsov wrote: > On 2016/11/23 05:46:48, Felix Dahlke wrote: > > On 2016/11/22 15:39:35, Wladimir Palant wrote: > > > I'd still ask for reviewers here but accept empty input. Not being reminded > > > would be an unexpected side-effect of this option. > > > > I personally prefer to add reviewers via the web interface, but I can live > with > > prompting for reviewers without requiring them. Done. > > I share Felix's preference, probably because I'm basically using the same > workflow (upload, check that everything looks right via the web UI, add > reviewers, send notifications). I can live with the way it's implemented in the > patch set 2 but I'm not sure if this is really helpful. Yeah same here, I'd really prefer it if we didn't prompt for reviewers in that situation. How about we output a warning/message instead?
LGTM https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py#newcode81 hgreview.py:81: opts['reviewers'] = ui.prompt('Reviewers (comma-separated): ', '') On 2016/11/23 11:36:04, Felix Dahlke wrote: > Yeah same here, I'd really prefer it if we didn't prompt for reviewers in that > situation. How about we output a warning/message instead? I guess that I could live with a warning.
Here's a new patch set with the warning as discussed. https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29363768/diff/29363769/hgreview.py#newcode81 hgreview.py:81: opts['reviewers'] = ui.prompt('Reviewers (comma-separated): ', '') On 2016/11/23 15:42:10, Wladimir Palant wrote: > On 2016/11/23 11:36:04, Felix Dahlke wrote: > > Yeah same here, I'd really prefer it if we didn't prompt for reviewers in that > > situation. How about we output a warning/message instead? > > I guess that I could live with a warning. Alright, gave that a go :) It's not incredibly intrusive, people might miss it. Still, IMHO it's the best option since I figure people that wan't to double check the review before sending out the mail will manage to double check that they have added reviewers, too! Here's how the output looks: No reviewers specified, edit the review to add some. Upload server: https://codereview.adblockplus.org (change with -s/--server) Your browser has been opened to visit: https://codereview.adblockplus.org/get-access-token?port=8001 If your browser is on a different machine then exit and re-run upload.py with the command-line parameter --no_oauth2_webbrowser Issue created. URL: https://codereview.adblockplus.org/29364056 Uploading base file for hgreview.py
LGTM
https://codereview.adblockplus.org/29363768/diff/29364059/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29363768/diff/29364059/hgreview.py#newcode81 hgreview.py:81: print 'No reviewers specified, edit the review to add some.' Please use ui.status() rather than print, this will make sure that the output is suppressed if -q is specified.
New patch set is up. https://codereview.adblockplus.org/29363768/diff/29364059/hgreview.py File hgreview.py (right): https://codereview.adblockplus.org/29363768/diff/29364059/hgreview.py#newcode81 hgreview.py:81: print 'No reviewers specified, edit the review to add some.' On 2016/11/23 16:30:14, Wladimir Palant wrote: > Please use ui.status() rather than print, this will make sure that the output is > suppressed if -q is specified. Done.
LGTM
LGTM 
|
