chromium-status: automatically linkify usernames/status messages

status.py

 # coding=utf-8
 # Copyright (c) 2012 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.
 
 """Status management pages."""
 
 import datetime
 import json
 import re
 
 from google.appengine.api import memcache
 from google.appengine.ext import db
 
 from base_page import BasePage
 import utils
 
 
 ALLOWED_ORIGINS = [
     'https://gerrit-int.chromium.org',
     'https://gerrit.chromium.org',
 ]
 
 
+class Link(object):

[Vadim Sh., 2013/12/03 20:06:36]

I'd convert it to collections.namedtuple. Shorter, immutable, __repr__ for free.

Up to you, class is also OK.

[Vadim Sh., 2013/12/03 20:06:36]

Also, 'Link' is a misleading name. This object can represent a chunk of a plain
text, not necessary a link.

[vapier, 2013/12/04 07:54:33]

we discussed collections in the previous patch.  afaik, you can't do default
values using the collections module.  i was waiting for maruel to answer a
question and then based on that, either keep this code or switch to collections.

i can rename it to TextFragments.

+  """Simple object to hold text that might be linked"""
+
+  def __init__(self, text, target=None, is_email=False):
+    self.text = text
+    self.target = target
+    self.is_email = is_email
+
+  def __repr__(self):
+    return 'Link({%s->%s})' % (self.text, self.target)
+
+
+class LinkableText(object):
+  """Turns arbitrary text into a set of links"""
+
+  GERRIT_URLS = {
+      'chrome': 'https://chrome-internal-review.googlesource.com',
+      'chromium': 'https://chromium-review.googlesource.com',
+  }
+
+  WATERFALL_URLS = {
+      'chromeos': 'http://chromegw/i/chromeos',
+      'chromiumos': 'http://build.chromium.org/p/chromiumos',
+  }
+
+  # Automatically linkify convert known strings for the user.

[Vadim Sh., 2013/12/03 20:06:36]

nit: 'linkify convert'. Keep only one?)

[vapier, 2013/12/04 07:54:33]

done

+  _CONVERTS = [

[Vadim Sh., 2013/12/03 20:06:36]

Is is possible to get rid of 're.compile' repetition?

For instance,
_CONVERTERS = [(re.compile(x[0], flags=re.I),) + x[1:] for x in [
   (r'\b((http://)?((crbug|crosbug)(\.com)?(/(p/)?[0-9]+)))\b', ...),
   ...
]]

It also looks ugly :(

Another suggestion is to actually use 'bootstrap'.

@classmethod
def register_converter(cls, regex, target, pretty, is_email):
  cls._CONVERTERS.append((re.compile(regex, ...), ...))

@classmethod
def bootstrap(cls):
  cls.register_converter(....)
  cls.register_converter(....)

I actually like it more:
a) It adds a proper documentation to format of _CONVERTERS tuples.
b) It gives an example of what subclasses should do in 'bootstrap'.

It's optional. Current approach is also fine.

[vapier, 2013/12/04 07:54:33]

it is a bit cleaner, and it would provide self documentation.  i'm not sure
about the overhead difference, but i think that's generally a non-issue as
appspot will warmup things ahead of time when load gets to a certain point, and
the bootstrap code only runs once then.

i'm a fan of giant static data lists, but i'll see about converting to your
version.

+      # Convert CrOS bug links.  Support the forms:
+      # http://crbug.com/1234
+      # http://crosbug.com/1234
+      # crbug/1234
+      # crosbug/p/1234
+      (re.compile(
+           #   1   2      3      4        5       6 7
+           r'\b((http://)?((crbug|crosbug)(\.com)?(/(p/)?[0-9]+)))\b',
+           flags=re.I),
+       r'http://\4.com\6', r'\1', False),
+
+      # Convert e-mail addresses.
+      (re.compile(r'(([-+.a-z0-9_!#$%&*/=?^_`{|}~]+)@[-a-z0-9.]+\.[a-z0-9]+)\b',
+                  flags=re.I),
+       r'\1', r'\2', True),

[Vadim Sh., 2013/12/03 20:06:36]

Why not put 'mailto:' into target (i.e. r'mailto:\1') and remove is_email
completely (from _CONVERTERS and Link and html).

[vapier, 2013/12/04 07:54:33]

"mailto:" is part of the rendering side (i.e. HTML) not part of the data side
(i.e. this) and people wanted me to keep them completely sep

+
+      # Convert SHA1's to gerrit links.  Assume all external since
+      # there is no sane way to detect it's an internal CL.
+      (re.compile(r'\b([0-9a-f]{40})\b', flags=re.I),
+       r'%s/#q,\1,n,z' % GERRIT_URLS['chromium'], r'\1', False),
+
+      # Convert public gerrit CL numbers.
+      (re.compile(r'\b(CL:([0-9]+))\b', flags=re.I),
+       r'%s/\2' % GERRIT_URLS['chromium'], r'\1', False),
+      # Convert internal gerrit CL numbers.
+      (re.compile(r'\b(CL:\*([0-9]+))\b', flags=re.I),
+       r'%s/\2' % GERRIT_URLS['chrome'], r'\1', False),
+
+      # Do this for everyone since "cbuildbot" is unique to CrOS.
+      # Otherwise, we'd do this in the bootstrap func below.
+      # Match the string:
+      #   Automatic: "cbuildbot" on "x86-generic ASAN" from.
+      (re.compile(r'("cbuildbot" on "([^"]+ canary)")',
+                  flags=re.I),
+       r'%s/builders/\2' % WATERFALL_URLS['chromeos'], r'\1', False),
+      (re.compile(r'("cbuildbot" on "([^"]+)")',
+                  flags=re.I),
+       r'%s/builders/\2' % WATERFALL_URLS['chromiumos'], r'\1', False),
+  ]
+
+  @classmethod
+  def bootstrap(cls, _app_name):
+    """Add conversions specific to |app_name| instance"""
+    pass
+
+  @classmethod
+  def parse(cls, text):
+    """Creates a list of Link objects based on |text|"""
+    if not text:
+      return []
+    for prog, target, pretty_text, is_email in cls._CONVERTS:
+      m = prog.search(text)
+      if m:
+        link = Link(m.expand(pretty_text),
+                    target=m.expand(target),
+                    is_email=is_email)
+        left_links = cls.parse(text[:m.start()].rstrip())
+        right_links = cls.parse(text[m.end():].lstrip())
+        return left_links + [link] + right_links

[Vadim Sh., 2013/12/03 20:06:36]

This looks nice and elegant :)

+    return [Link(text)]
+
+  def __init__(self, text):
+    self.raw_text = text
+    self.links = self.parse(text.strip())
+
+  def __str__(self):
+    return self.raw_text
+
+
 class Status(db.Model):
   """Description for the status table."""
   # The username who added this status.
   username = db.StringProperty(required=True)
   # The date when the status got added.
   date = db.DateTimeProperty(auto_now_add=True)
   # The message. It can contain html code.
   message = db.StringProperty(required=True)
 
+  def __init__(self, *args, **kwargs):
+    # Normalize newlines otherwise the DB store barfs.
+    kwargs['message'] = kwargs.get('message', '').replace('\n', '')
+
+    super(Status, self).__init__(*args, **kwargs)
+    self.username_links = LinkableText(self.username)

[Vadim Sh., 2013/12/03 20:06:36]

__init__ in db.Model subclass is not very conventional. Afaik, there's no
guarantee that db.get(...) or Query.fetch(...) is actually calling it with
**kwargs set properly (and not modifying it afterwards).

Also Status object is mutable, 'message' may change after constructor, and
'message_links' will become out of date.

I propose the following change:
1) Store 'message' as is (by adding multiline=True to db.StringProperty). Remove
'\n' right before displaying it (or when converting it to LinkableText).
2) Convert username_links & message_links to @property, compute it on the fly.
3) Remove __init__.

What do you think?

[vapier, 2013/12/04 07:54:33]

for the message issue, i think this would fix it:
 def __init__(self, message=None, *args, **kwargs):

i'm not sure you can change the db model on the fly (there will already be
entities with these in there).  also, i don't think we want to allow multiline
... see the comments in the previous patchset where we discussed this issue.

the @property idea SGTM

+    self.message_links = LinkableText(self.message)
+
   @property
   def general_state(self):
     """Returns a string representing the state that the status message
     describes.
     """
     message = self.message
     closed = re.search('close', message, re.IGNORECASE)
     if closed and re.search('maint', message, re.IGNORECASE):
       return 'maintenance'
     if re.search('throt', message, re.IGNORECASE):
@@ skipping 245 matching lines @@
       return self._handle(error_message, last_message)
     else:
       put_status(Status(message=new_message, username=self.user.email()))
       self.redirect("/")
 
 
 def bootstrap():
   # Guarantee that at least one instance exists.
   if db.GqlQuery('SELECT __key__ FROM Status').get() is None:
     Status(username='none', message='welcome to status').put()
+  LinkableText.bootstrap(BasePage.APP_NAME)