chromium-status: automatically linkify usernames/status messages

status.py

Index: status.py
diff --git a/status.py b/status.py
index fc37e40ea896dcbff03fe3c5f8f14ea01ca3670c..2f8a00aa6ad9bdf4b36c40304fd7da1c2c8988ff 100644
--- a/status.py
+++ b/status.py
@@ -22,6 +22,101 @@ ALLOWED_ORIGINS = [
 ]
 
 
+class Link(object):
+  """Simple object to hold text that might be linked"""

[M-A Ruel, 2013/11/25 16:18:14]

"""Holds text ..

No need to state a relative quality of the object in the docstring, here that it
is simple.

[vapier, 2013/11/25 17:51:26]

i agree it's simple, but i find scanning a one line docstring is a lot faster
than scanning the content

+
+  def __init__(self, text, target=None, is_email=False):

[M-A Ruel, 2013/11/25 16:18:14]

I'd prefer to not use default arguments.

[vapier, 2013/11/25 17:51:26]

any reason why ?  the intention is for email to be uncommon.

if you're really set on there not being defaults, then i might as well use the
python collections module and not create my own.

+    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):
+  """Turn arbitrary text into a set of links"""

[M-A Ruel, 2013/11/25 16:18:14]

Turns
(imperative everywhere)

[vapier, 2013/11/25 17:51:26]

i dont see the point, but i also dont really care to debate

+
+  GERRIT_URLS = {
+      'chrome': 'https://chrome-internal-review.googlesource.com',

[M-A Ruel, 2013/11/25 16:18:14]

Very hardcoded. :/ Could these be entities?

[vapier, 2013/11/25 17:51:26]

i have no idea what "entities" are.  plus this file already hardcodes gerrit
URLs, and the idea is to support known link styles across multiple instances (so
that CL:12345 works both in chromium-status and chromiumos-status and so on).

+      '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.
+  _CONVERTS = [
+      # 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),
+
+      # 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),
+  ]
+
+  @classmethod
+  def bootstrap(cls, _app_name):
+    """Add conversions specific to |app_name| instance"""
+    cls._CONVERTS += [
+      # Do this for everyone since "cbuildbot" is unique to CrOS.
+      # Match the string:
+      #   Automatic: "cbuildbot" on "x86-generic ASAN" from.
+      (re.compile(r'("cbuildbot" on "([^"]+ canary)")',

[M-A Ruel, 2013/11/25 16:18:14]

These should be stated as static lists. There isn't much dynamic in there,
unless you properly create entities to store the instance-specific rules, which
I prefer.

You can work around re.I by using r'(?i)' as a prefix to your regexp.

[vapier, 2013/11/25 17:51:26]

this was needed in a previous CL where i used a function callback.  i no longer
use that, so i no longer need to bootstrap these rules on the fly.

the re.compile/re.flags stuff aren't a problem to do at class decl time (as i'm
already doing that above).

+                  flags=re.I),
+       r'%s/builders/\2' % cls.WATERFALL_URLS['chromeos'], r'\1', False),
+      (re.compile(r'("cbuildbot" on "([^"]+)")',
+                  flags=re.I),
+       r'%s/builders/\2' % cls.WATERFALL_URLS['chromiumos'], r'\1', False),
+    ]
+
+  @classmethod
+  def parse(cls, text):
+    """Create 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
+    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.
@@ -31,6 +126,14 @@ class Status(db.Model):
   # The message. It can contain html code.
   message = db.StringProperty(required=True)
 
+  def __init__(self, *args, **kwargs):
+    # Normalize newlines otherwise the DB store barfs.

[M-A Ruel, 2013/11/25 16:18:14]

I'm fine with adding multiline=True instead. Actually,  I prefer that. The
constructor may not always be used.

[vapier, 2013/11/25 17:51:26]

i don't think we want people to insert newlines into their messages.  i'm not
sure the various parts of the system (or the clients) are ready to handle it,
and it doesn't make sense to even bother supporting it.

+    kwargs['message'] = kwargs.get('message', '').replace('\n', '')
+
+    super(Status, self).__init__(*args, **kwargs)
+    self.username_links = LinkableText(self.username)
+    self.message_links = LinkableText(self.message)
+
   @property
   def general_state(self):
     """Returns a string representing the state that the status message
@@ -246,6 +349,8 @@ class MainPage(BasePage):
 
   def _handle(self, error_message='', last_message=''):
     """Sets the information to be displayed on the main page."""
+    LinkableText.bootstrap(self.APP_NAME)

[M-A Ruel, 2013/11/25 16:18:14]

This is a bad idea to do this there. It creates a memory leak as
LinkableText._CONVERT grows on each root page rendering. There's a bootstrap
function at the end to manage on-startup issues like that.

[vapier, 2013/11/25 17:51:26]

i'm not terribly familiar with the app engine model.  i could make it so the
func itself handles being called more than once.

i also had it here because i thought the app_name stuff was initialized later,
and to use the api as available.  but i guess that is setup in the basepage's
bootstrap phase, and is set on the class level, so i can just use that directly.

+
     try:
       limit = min(max(int(self.request.get('limit')), 1), 1000)
     except ValueError: