On Wed, Feb 10, 2010 at 10:49 PM, Karl Fogel <email address hidden> wrote:
> Karl Fogel has proposed merging lp:~kfogel/launchpad/cc-script-new-world into lp:launchpad/devel.
>
>    Requested reviews:
>    Jonathan Lange (jml)
>

Hey Karl,

The change looks great. Most of my comments on the code are related to
Python idioms.

If this were application code, I might push a bit harder on whether
the ExCon data structure is the best way of thinking about the
problem. As it is, it's not application code and I've run out of
neurons for the morning :)

>
> Various improvements to utilities/community-contributions.py.  Namely:
>
>  1. Show Canonical developers too (those not on the Launchpad team)
>  2. Don't show huge commit blocks; just elide and link to the top rev.
>  3. Finally clean up various utf-8 encoding issues (bug #432742)
>  4. Add a '--draft-run' option for testing.
>
> You can see the results here:
>
>  https://dev.launchpad.net/Contributions/Draft
>
> (Compare with https://dev.launchpad.net/Contributions for old output.)
>

The new output is much better.

> === modified file 'utilities/community-contributions.py'
> --- utilities/community-contributions.py        2010-02-08 14:38:31 +0000
> +++ utilities/community-contributions.py        2010-02-10 22:49:18 +0000
...
> @@ -56,85 +57,133 @@
>     sys.exit(1)
>
>
> -# While anyone with "@canonical.com" in their email address will be
> -# counted as a Canonical contributor, sometimes Canonical people
> -# submit from personal addresses, so we still need a list.
> +# The output contains two classes of contributors: people who don't
> +# work for Canonical at all, and people who do work for Canonical but
> +# not on the Launchpad team.

As wgrant hinted on IRC, it's worth being explicit about what we do
with Launchpad team alumnists (e.g. Carlos & Daniel Silverstone).

>  #
>  # XXX: Karl Fogel 2009-09-10 bug=513608: We should use launchpadlib
> -# to consult Launchpad itself to find out who's a Canonical developer.
> -known_canonical_devs = (
> -    u'Aaron Bentley',
> -    u'Abel Deuring',
> -    u'Adam Conrad',
> -    u'Andrew Bennetts',
> -    u'Anthony Lenton',
> -    u'Barry Warsaw',
> -    u'Brad Crittenden',
> -    u'Carlos Perello Marin',
> -    u'Carlos Perelló Marín',
> -    u'Celso Providelo',
> -    u'Christian Robottom Reis',
> -    u'Cody Somerville',
> -    u'Curtis Hovey',
> -    u'Dafydd Harries',
> -    u'Daniel Silverstone',
> -    u'Danilo Šegan',
> -    u'Данило Шеган',
> -    u'данило шеган',
> -    u'David Allouche',
> -    u'Deryck Hodge',
> -    u'Diogo Matsubara',
> -    u'Elliot Murphy',
> -    u'Francis J. Lacoste',
> -    u'Gabriel Neuman <email address hidden>',
> -    u'Gary Poster',
> -    u'Guilherme Salgado',
> -    u'Gustavo Niemeyer',
> -    u'Henning Eggers',
> -    u'Herb McNew',
> -    u'James Henstridge',
> -    u'Jelmer Vernooij',
> -    u'Jeroen Vermeulen',
> -    u'Jonathan Knowles',
> -    u'Jonathan Lange',
> -    u'Julian Edwards',
> -    u'Karl Fogel',
> -    u'Kees Cook',
> -    u'Launch Pad',
> -    u'Launchpad Developers',
> -    u'LaMont Jones',
> -    u'Leonard Richardson',
> -    u'Malcolm Cleaton',
> -    u'Maris Fogels',
> -    u'Martin Albisetti',
> -    u'Martin Pool',
> -    u'Matt Zimmerman',
> -    u'Matthew Revell',
> -    u'Michael Hudson',
> -    u'Michael Nelson',
> -    u'Muharem Hrnjadovic',
> -    u'Patch Queue Manager',
> -    u'Paul Hummer',
> -    u'Robert Collins',
> -    u'Sidnei',
> -    u'Sidnei da Silva',
> -    u'Steve Kowalik',
> -    u'Steve McInerney',
> -    u'Stuart Bishop',
> -    u'Tom Berger',
> -    u'david',
> -    <email address hidden>',
> -    u'kiko@beetle',
> -    )
> +# to consult Launchpad itself to find out who's a Canonical developer,
> +# and within that who's a Launchpad developer.
> +
> +# People on the Canonical Launchpad team.
> +known_canonical_lp_devs = \
> +    [x.encode('utf-8', 'xmlcharrefreplace') \

The backslashes here are unnecessary. Python "lines" run on if there's
an unclosed bracket.

> +         for x in (u'Aaron Bentley',
> +                   u'Abel Deuring',
> +                   u'Andrew Bennetts',
> +                   u'Barry Warsaw',
> +                   u'Bjorn Tillenius',
> +                   u'Björn Tillenius',
> +                   u'Brad Crittenden',
> +                   u'Brian Fromme',
> +                   u'Celso Providelo',
> +                   u'Christian Reis',
> +                   u'kiko {_AT_} beetle',
> +                   u'Cody Somerville',
> +                   u'Cody A.W. Somerville',
> +                   u'Curtis Hovey',
> +                   u'Dafydd Harries',
> +                   u'Danilo Šegan',
> +                   u'Данило Шеган',
> +                   u'данило шеган',
> +                   u'David Murphy',
> +                   u'Deryck Hodge',
> +                   u'Diogo Matsubara',
> +                   u'Edwin Grubbs',
> +                   u'Elliot Murphy',
> +                   u'Francis Lacoste',
> +                   u'Francis J. Lacoste',
> +                   u'Gary Poster',
> +                   u'Gavin Panella',
> +                   u'Graham Binns',
> +                   u'Guilherme Salgado',
> +                   u'Henning Eggers',
> +                   u'James Henstridge',
> +                   u'Jelmer Vernooij',
> +                   u'Jeroen Vermeulen',
> +                   u'Jeroen T. Vermeulen',
> +                   u'Joey Stanford',
> +                   u'John Lenton',
> +                   u'Jonathan Lange',
> +                   u'jml {_AT_} canonical.com',
> +                   u'jml {_AT_} mumak.net',
> +                   u'Julian Edwards',
> +                   u'Karl Fogel',
> +                   u'Launchpad APA',
> +                   u'Launchpad Patch Queue Manager',
> +                   u'Launchpad PQM Bot',
> +                   u'Leonard Richardson',
> +                   u'Maris Fogels',
> +                   u'Mark Shuttleworth',
> +                   u'Martin Albisetti',
> +                   u'Martin Pool',
> +                   u'Matt Zimmerman',
> +                   u'Matthew Paul Thomas',
> +                   u'Matthew Revell',
> +                   u'matthew.revell {_AT_} canonical.com',
> +                   u'Michael Casadevall',
> +                   u'Michael Hudson',
> +                   u'Michael Nelson',
> +                   u'Muharem Hrnjadovic',
> +                   u'muharem {_AT_} canonical.com',
> +                   u'Paul Hummer',
> +                   u'Robert Collins',
> +                   u'Sidnei da Silva',
> +                   u'Stuart Bishop',
> +                   u'Steve McInerney',
> +                   u'<steve {_AT_} stedee.id.au>',
> +                   u'Tom Haddon',
> +                   u'Tim Penhey',
> +                   u'Tom Berger',
> +                   u'Ursula Junque',
> +                   )]
> +
> +# People known to work for Canonical but not on the Launchpad team.
> +# Anyone with "@canonical.com" in their email address is considered to
> +# work for Canonical, but some people occasionally submit changes from
> +# their personal email addresses; this list contains people known to
> +# do that, so we can treat them appropriately in the output.
> +known_canonical_non_lp_devs = \
> +    [x.encode('utf-8', 'xmlcharrefreplace') \

Likewise here.

This encoding pattern is repeated an awful lot. Your call, but I think
it would make sense to define a function like:

    def encode_for_wiki(x):
        """Encode a unicode string for display on the wiki."""
        return x.encode('utf-8', 'xmlcharrefreplace')

and to use that.

> +         for x in (u'Adam Conrad',
> +                   u'Andrew Bennetts',
> +                   u'Anthony Lenton',
> +                   u'Carlos Perello Marin',
> +                   u'Carlos Perelló Marín',
> +                   u'Cody Somerville',
> +                   u'Daniel Silverstone',
> +                   u'David Allouche',
> +                   u'Elliot Murphy',
> +                   u'Gabriel Neuman gneuman {_AT_} async.com',
> +                   u'Gustavo Niemeyer',
> +                   u'James Henstridge',
> +                   u'Jonathan Knowles',
> +                   u'Kees Cook',
> +                   u'LaMont Jones',
> +                   u'Malcolm Cleaton',
> +                   u'Martin Pool',
> +                   u'Matt Zimmerman',
> +                   u'Steve Kowalik',
> +                   )]
>
>  # Some people have made commits using various names and/or email
>  # addresses, so this map will be used to merge them accordingly.
> -merge_names_map = {
> -    u'Jamal Fanaian <email address hidden>':
> -        u'Jamal Fanaian <email address hidden>',
> -    u'Jamal Fanaian <jamal@jfvm1>':
> -        u'Jamal Fanaian <email address hidden>',
> -    }
> +# The map is initialized from this list of pairs, where each pair is
> +# of the form (CONTRIBUTOR_AS_SEEN, UNIFYING_IDENTITY_FOR_CONTRIBUTOR).
> +merge_names_pairs = (
> +    (u'Jamal Fanaian <jfanaian {_AT_} gmail.com>',
> +     u'Jamal Fanaian <jamal.fanaian {_AT_} gmail.com>'),
> +    (u'Jamal Fanaian <jamal {_AT_} jfvm1>',
> +     u'Jamal Fanaian <jamal.fanaian {_AT_} gmail.com>'),
> +    (u'LaMont Jones <lamont {_AT_} rover3>',
> +     u'LaMont Jones <lamont {_AT_} debian.org>'),
> +    )
> +# Then put it in dictionary form with the correct encodings.
> +merge_names_map = { }
> +for pair in merge_names_pairs:
> +    merge_names_map[pair[0].encode('utf-8', 'xmlcharrefreplace')] = \
> +        pair[1].encode('utf-8', 'xmlcharrefreplace')
> +

This can be written a little better:

merge_names_map = {}
for as_seen, identifier in merge_names_pairs:
    merge_names_map[encode_for_wiki(as_seen)] = encode_for_wiki(identifier)

The differences are:
  * No space between the braces when defining empty dict
  * Use tuple unpacking to avoid thinking about indexing of the tuple
in the loop
  * Using the encode_for_wiki helper.

Alternatively, you could do this:

merge_names_map = dict((encode_for_wiki(a), encode_for_wiki(b)) for a,
b in merge_names_pairs)

But some would consider that too functional to be readable by a
general audience. (I don't!)

>
>  class ContainerRevision():
>     """A wrapper for a top-level LogRevision containing child LogRevisions."""
> @@ -187,14 +236,22 @@
>         # will give you some information about it before you click
>         # (because a rev id often identifies the committer).
>         rev_id_url = rev_url_base + rev_id
> +
> +        if len(self.contained_revs) <= 10:
> +            commits_block = "\n ".join(
> +                ["[[%s|%s]]" % (rev_url_base + lr.rev.revision_id, lr.revno)
> +                 for lr in self.contained_revs])
> +        else:
> +            commits_block = "''see the [[%s|full revision]] for details " \
> +                "(it contains %d commits)''" \
> +                % (rev_id_url, len(self.contained_revs))
> +

It might be nicer to write this as:
  commits_block = (
      "''see the [[%s|full revision]] for details (it contains %d commits)''"
      % (rev_id_url, len(self.contained_revs)))

>         text = [
>             " * [[%s|r%s]] -- %s\n" % (rev_id_url, self.top_rev.revno,
>                                        date_str),
>             " {{{\n%s\n}}}\n" % message,
>             " '''Commits:'''\n ",
> -            "\n ".join(["[[%s|%s]]" % (rev_url_base + lr.rev.revision_id,
> -                                       lr.revno)
> -                        for lr in self.contained_revs]),
> +            commits_block,
>             "\n",
>             ]
>         return ''.join(text)
> @@ -202,14 +259,15 @@
>
>  # "ExternalContributor" is too much to type, so I guess we'll just use this.
>  class ExCon():
> -    """A contributor to Launchpad from outside Canonical."""
> -
> -    def __init__(self, name):
> -        """Create a new external contributor named NAME.  NAME is usually
> -        e.g. "Veronica Random <email address hidden>", but any "@"-sign
> -        will be disguised in the new object."""
> -
> -        self.name = name.replace("@", " {_AT_} ")
> +    """A contributor to Launchpad from outside Canonical's Launchpad team."""
> +
> +    def __init__(self, name, is_canonical=False):
> +        """Create a new external contributor named NAME.

Blank line here please.

> +        If IS_CANONICAL is True, then this is a contributor from
> +        within Canonical, but not on the Launchpad team at Canonical.
> +        NAME is something like "Veronica Random <vr {_AT_} example.com>"."""

Newline before closing triple quote please.

Also, we tend to single quote parameter names rather than capitalize them.

> +        self.name = name
> +        self.is_canonical = is_canonical
>         # If name is "Veronica Random <veronica {_AT_} example.com>",
>         # then name_as_anchor will be "veronica_random".
>         self.name_as_anchor = \
> @@ -231,10 +289,13 @@
>     def show_contributions(self):
>         "Return a wikified string showing this contributor's contributions."
>         plural = "s"
> +        name = self.name
> +        if self.is_canonical:
> +            name = name + " (Canonical developer)"
>         if self.num_landings() == 1:
>             plural = ""
>         text = [
> -            "== %s ==\n\n" % self.name,
> +            "=== %s ===\n\n" % name,
>             "''%d top-level landing%s:''\n\n" % (self.num_landings(), plural),
>             ''.join(map(str, sorted(self._landings,
>                                     key=lambda x: x.top_rev.revno,
> @@ -252,30 +313,58 @@
>     ALL_EX_CONS is a dictionary mapping author names (as received from
>     the bzr logs, i.e., with email address undisguised) to ExCon objects.
>     """
> +
> +    # If a contributor's address contains this, then they are a
> +    # Canonical developer -- maybe on the Launchpad team, maybe not.
> +    #
> +    # (It'd be nice to have the equivalent of a C static variable, so
> +    # this doesn't have to get reinitialized on each entry.  We could
> +    # always abuse the fact that keyword parameters to functions are
> +    # evaluated only once, at function definition time... but that
> +    # hardly seems like playing by the Marquess of Queensberry Rules.)

Some thoughts:
  * He was a Marquis, not a Marquess.
  * Evaluating keyword parameters to functions is often used for
getting at compiled regexen, for example.
  * You could define it as a module-level variable.

FWIW, I tend to capitalize variables that ought to be constants. e.g.
CANONICAL_ADDR,

> +    canonical_addr = \
> +        u" {_AT_} canonical.com".encode('utf-8', 'xmlcharrefreplace')
> +
>     ex_cons_this_rev = []
> -    for a in authors:
> -        known = False
> -        for name_fragment in known_canonical_devs:
> -            if <email address hidden>" in a or name_fragment in a:
> -                known = True
> +    for author in authors:
> +        known_canonical_lp_dev = False
> +        known_canonical_non_lp_dev = False
> +        # The authors we list in the source code have their addresses
> +        # disguised (since this source code is public).  We must
> +        # disguise the ones coming from the Bazaar logs in the same way,
> +        # so string matches will work.
> +        author = author.encode('utf-8', 'xmlcharrefreplace')

There's that thing again.

> +        author = author.replace("@", " {_AT_} ")
> +        for name_fragment in known_canonical_lp_devs:
> +            if name_fragment in author:
> +                known_canonical_lp_dev = True
>                 break
> -        if not known:
> -            # Use the merge names map to merge contributions from the same
> -            # person using alternate names and/or emails.
> -            if a in merge_names_map:
> -                a = merge_names_map[a]
> -
> -            ### There's a variant of the Singleton pattern that could be
> -            ### used for this, whereby instantiating an ExCon object would
> -            ### just get back an existing object if such has already been
> -            ### instantiated for this name.  But that would make this code
> -            ### non-reentrant, and that's just not cool.
> -            if a in all_ex_cons:
> -                ec = all_ex_cons[a]
> -            else:
> -                ec = ExCon(a)
> -                all_ex_cons[a] = ec
> -            ex_cons_this_rev.append(ec)
> +        if known_canonical_lp_dev:
> +            continue
> +

I think it's worth putting a comment here about why we are skipping
them. By the time I got to this point I had forgotten that ExCon meant
External Contributor and it took me a while to realize that it means
"external to Canonical Launchpad" not "external to Canonical".

Putting the logic is figure out 'known_canonical_lp_dev' into a
function would also help.

> +        # Use the merge names map to merge contributions from the same
> +        # person using alternate names and/or emails.
> +        author = merge_names_map.get(author, author)
> +
> +        if canonical_addr in author:
> +            known_canonical_non_lp_dev = True
> +        else:
> +            for name_fragment in known_canonical_non_lp_devs:
> +                if name_fragment in author:
> +                    known_canonical_non_lp_dev = True
> +                    break
> +
> +        ### There's a variant of the Singleton pattern that could be
> +        ### used for this, whereby instantiating an ExCon object would
> +        ### just get back an existing object if such has already been
> +        ### instantiated for this name.  But that would make this code
> +        ### non-reentrant, and that's just not cool.

Three comment characters, really?

> +        if author in all_ex_cons:
> +            ec = all_ex_cons[author]
> +        else:
> +            ec = ExCon(author, is_canonical=known_canonical_non_lp_dev)
> +            all_ex_cons[author] = ec

This could also be written as:

  ec = all_ex_cons.get(author, None)
  if ec is None:
      ec = ExCon(author, is_canonical=known_canonical_non_lp_dev)
      all_ex_cons[author] = ec


> @@ -344,10 +462,11 @@
>
>
>  page_intro = """This page shows contributions to Launchpad from \
> -outside Canonical.  It only lists changes that have landed in the \
> -Launchpad ''devel'' tree, so changes that land in ''db-devel'' first \
> -may take a while to show up (see the [[Trunk|trunk explanation]] for \
> -more).
> +developers not on the Launchpad team at Canonical.
> +
> +It only lists changes that have landed in the Launchpad ''devel'' \
> +tree, so changes that land in ''db-devel'' first may take a while to \
> +show up (see the [[Trunk|trunk explanation]] for more).

You only need the backslashes here if you want to avoid inserting
newlines into the string.

jml