Don't send mail for policy_checklist comments.

handlers/policy_checklist/handler.py

 # Copyright (c) 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Handler for chrome/app/policy/policy_templates.json
 
 This handler examines changes to the policy_templates.json file and posts
 comments with checklists for the patch author and reviewer to go through to
 avoid common pitfalls.
 """
@@ skipping 13 matching lines @@
 
 REVIEW_MESSAGE_TEMPLATE = 'review_message.txt'
 ADDITION_COMMENT_TEMPLATE = 'addition_comment.txt'
 MODIFICATION_COMMENT_TEMPLATE = 'modification_comment.txt'
 
 
 JINJA_ENVIRONMENT = jinja2.Environment(
     loader=jinja2.FileSystemLoader(os.path.dirname(__file__)))
 
 
+def prepare_address_list(addr, email_list):
+  """Prepares |email_list| for use as rietveld query parameter.
+
+  Canonicalizes the entries in |email_list|, removes any occurrences of |addr|,
+  joins the entries with commas and returns the result.
+  """
+  return ','.join([item for canonicalized in map(util.get_emails, email_list)
+                   for item in canonicalized

[agable, 2013/08/27 14:49:19]

Double-nested fors in list comprehensions are ugly and imho unreadable. I'd
suggest turning this into two comprehensions for the sake of readability.

[Mattias Nissler (ping if slow), 2013/08/27 15:08:41]

Note that the nesting also flattens the list of lists, which I don't see how to
accomplish in two separate comprehensions. I've now switched to an
itertools.chain-based solution, I hope that is clearer.

[agable, 2013/08/27 15:56:26]

Ah, I'd missed the nesting from get_emails. Seems the rietveld is guaranteed to
return a list of strings, where each string is one reviewer. It would make more
sense to me to do a single list comprehension with "util.clean_email" which is
just a slightly different passthrough to email.utils.getaddresses (that asserts
it only gets back a single address, for example).

[Mattias Nissler (ping if slow), 2013/08/27 16:07:23]

Done.

+                   if item != addr])
+
+
 def process(addr, message, review, rietveld):
   """Handles reviews for chrome/app/policy/policy_templates.json.
 
   This looks at the patch to identify additions/modifications to policy
   definitions and posts comments with a checklist intended for the author and
   reviewer to go through in order to catch common mistakes.
   """
 
   if POLICY_TEMPLATES_FILE not in review.latest_patchset.files:
     return
@@ skipping 27 matching lines @@
       continue
 
     rietveld.add_inline_comment(
         review.issue_id, review.latest_patchset.patchset,
         review.latest_patchset.files[POLICY_TEMPLATES_FILE].id,
         line, side, template.render(review=review, chunk=chunk))
 
   # Finally, post all inline comments.
   if len(chunks) > 0:
     template = JINJA_ENVIRONMENT.get_template(REVIEW_MESSAGE_TEMPLATE)
-    rietveld.post_comment(review.issue_id, template.render(review=review), True)
+    rietveld.publish_inline_comments(
+        review.issue_id, template.render(review=review),
+        prepare_address_list(addr, review.issue_data.reviewers),
+        prepare_address_list(addr, review.issue_data.cc),
+        send_mail=False)

[agable, 2013/08/27 14:49:19]

send_mail defaults to False.

[Mattias Nissler (ping if slow), 2013/08/27 15:08:41]

Done.