Fixes 4662 - [hgreview] Add an option for suppressing the email after upload

hgreview.py

 import BaseHTTPServer
 import os
 import re
 import socket
 import sys
 import urllib
 
 from mercurial import cmdutil, error
 
 SERVER = 'https://codereview.adblockplus.org'
 UPLOADTOOL_URL = SERVER + '/static/upload.py'
 
 cmdtable = {}
 command = cmdutil.command(cmdtable)
 
 
 @command('review',
          [
              ('i', 'issue', '', 'If given, adds a patch set to this review, otherwise create a new one.', 'ISSUE'),
              ('r', 'revision', '', 'Revision to diff against or a revision range to upload.', 'REV'),
              ('c', 'change', '', 'A single revision to upload.', 'REV'),
              ('t', 'title', '', 'New review subject or new patch set title.', 'TITLE'),
              ('m', 'message', '', 'New review description or new patch set message.', 'MESSAGE'),
              ('w', 'reviewers', '', 'Add reviewers (comma separated email addresses or @adblockplus.org user names).', 'REVIEWERS'),
              ('', 'cc', '', 'Add CC (comma separated email addresses or @adblockplus.org user names).', 'CC'),
              ('', 'private', None, 'Make the review restricted to reviewers and those CCed.'),
              ('y', 'assume_yes', None, 'Assume that the answer to yes/no questions is \'yes\'.'),
              ('', 'print_diffs', None, 'Print full diffs.'),
-             ('', 'no_mail', None, 'Don\'t send an email after uploading.', 'NO_MAIL'),

[Wladimir Palant, 2016/11/22 15:39:35]

This is a flag without any parameter - please drop 'NO_MAIL'.

[Felix Dahlke, 2016/11/23 05:46:48]

Done.

+             ('', 'no_mail', None, 'Don\'t send an email after uploading.'),
          ], '[options] [path...]')
 def review(ui, repo, *paths, **opts):
     '''
       Uploads a review to https://codereview.adblockplus.org/ or updates an
       existing review request. This will always send mails for new reviews, when
       updating a review mails will only be sent if a message is given.
     '''
     args = ['--oauth2', '--server', SERVER]
     if ui.debugflag:
         args.append('--noisy')
     elif ui.verbose:
         args.append('--verbose')
     elif ui.quiet:
         args.append('--quiet')
 
-    if (not opts.get("no_mail") and

[Vasily Kuznetsov, 2016/11/22 15:29:35]

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).

[Felix Dahlke, 2016/11/23 05:46:48]

Whoops, something I'll have to get used to :P Done.

+    if (not opts.get('no_mail') and
         (not opts.get('issue') or opts.get('message'))):

[Vasily Kuznetsov, 2016/11/22 15:29:35]

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.

[Felix Dahlke, 2016/11/23 05:46:48]

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.

[Vasily Kuznetsov, 2016/11/23 11:00:28]

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.

         args.append('--send_mail')
 
     if opts.get('revision') and opts.get('change'):
         raise error.Abort('Ambiguous revision range, only one of --revision and --change can be specified.')
     if opts.get('change'):
         rev = repo[opts['change']]
         args.extend(['--rev', '{}:{}'.format(rev.parents()[0], rev)])
     elif opts.get('revision'):
         args.extend(['--rev', opts['revision']])
     else:
@@ skipping 11 matching lines @@
         if not opts.get('message'):
             opts['message'] = opts['title']
 
         path = (ui.config('paths', 'default-push')
                 or ui.config('paths', 'default')
                 or '')
         match = re.search(r'^(?:https://|ssh://hg@)(.*)', path)
         if match:
             opts['base_url'] = 'https://' + match.group(1)
 
-        if not opts.get('no_mail'):
-            # Make sure there is at least one reviewer
-            if not opts.get('reviewers'):
-                opts['reviewers'] = ui.prompt('Reviewers (comma-separated): ', '')

[Vasily Kuznetsov, 2016/11/22 15:29:35]

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.

[Wladimir Palant, 2016/11/22 15:39:35]

I'd still ask for reviewers here but accept empty input. Not being reminded
would be an unexpected side-effect of this option.

[Felix Dahlke, 2016/11/23 05:46:48]

I personally prefer to add reviewers via the web interface, but I can live with
prompting for reviewers without requiring them. Done.

[Felix Dahlke, 2016/11/23 05:46:48]

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

[Vasily Kuznetsov, 2016/11/23 11:00:28]

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.

[Felix Dahlke, 2016/11/23 11:36:04]

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?

[Wladimir Palant, 2016/11/23 15:42:10]

I guess that I could live with a warning.

[Felix Dahlke, 2016/11/23 16:06:03]

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

-            if not opts['reviewers'].strip():
-                raise error.Abort('No reviewers given.')
+        # Make sure there is at least one reviewer
+        if not opts.get('reviewers'):
+            if opts.get('no_mail'):
+                ui.status('No reviewers specified, edit the review to add '
+                          'some.\n')
+            else:
+                opts['reviewers'] = ui.prompt('Reviewers (comma-separated): ',
+                                              '')
+                if not opts['reviewers'].strip():
+                    raise error.Abort('No reviewers given.')
 
     for opt in ('reviewers', 'cc'):
         if opts.get(opt):
             users = [u if '@' in u else u + '@adblockplus.org'
                      for u in re.split(r'\s*,\s*', opts[opt])]
             opts[opt] = ','.join(users)
 
     for opt in ('issue', 'title', 'message', 'reviewers', 'cc', 'base_url'):
         if opts.get(opt, ''):
             args.extend(['--' + opt, opts[opt]])
@@ skipping 58 matching lines @@
   </body>
 </html>
 ''' % port
 
     # Run the upload tool
     issue, patchset = scope['RealMain']([upload_path] + args)
 
     # Wait for the page to check in and retrieve issue URL
     if server:
         server.handle_request()