Code review comment for lp://staging/~ceejatec/sfbugs2launchpad/user-mapping

Hi Chris,

lets me apologize that it needed ten days that somebody from the LP
engineering team took a look at your exciting work.

Aside from the new features which are very useful I appreciate very
much that you also wrote a good README file and added the Relax-NG
schema files.

I have a few comments on details of your changes (nothing serious,
see below), but my main concern is that the script is becoming
complex enough so that new features should be tested. (Well... the
entire code should be tested, but that's something for other branches ;)
If you don't have enough time to write some tests (which I would
understand quite well) I would file a bug roughly like "there is
great work waiting to be merged, once we have some tests" and
hopefully someone from the LP core team can take care of it soon.

And there is one blocker: Canonical requires that external
contributors sign a "Contributor License Agreement":
http://www.canonical.com/contributors . so, could you please sign it?

> @@ -224,14 +304,29 @@
> return date.isoformat() + 'Z'
>
>
> -def create_launchpad_issue_tree(issues, options):
> +def create_launchpad_issue_tree(issues, options, users):
> root = ET.Element("launchpad-bugs")
> root.attrib['xmlns'] = "https://launchpad.net/xmlns/2006/bugs"
>
> - def _sfuser(elem, name):
> - name = (name if name is not None else "noone").strip()
> - elem.text = name
> - elem.attrib["email"] = "%<email address hidden>" % elem.text
> + def _mapsfuser(elem, sfusername):
> + """
> + Populates the given element as a Launchpad "person", with the
> + full name as the text along with "email" and (optionally)
> + "name" attributes, where "name" is the Launchpad username. If
> + the sfusername is found in the "users" map, the values will
> + come from there; otherwise we'll make something up.
> + """
> + if (sfusername in users):

[My comment is perhaps caused by the fact that I had recently used
an object from the "surrounding function" in a local function
inadvertently...] I would prefer if users is passsed as a parameter
to _mapsfuser(). But consider this just as a suggestion ;)

> + userdata = users[sfusername]
> + elem.text = userdata.full_name
> + elem.attrib["email"] = userdata.email
> + if ("lp_user_name" in userdata):
> + elem.attrib["name"] = userdata.lp_user_name

I think lp_user_name should not appear in userdata, as such an element
is not specified in the new Relax-NG file sf-user-map.rnc. Also,
the email address provides equivalent information. So I think we don't
need the two lines above.

> + else:
> + # We have no source of information about this Sourceforge username
> + name = (sfusername if sfusername is not None else "noone").strip()
> + elem.text = name
> + elem.attrib["email"] = "%<email address hidden>" % elem.text
>
> for idx,issue in enumerate(issues):
> print "Handling issue %i/%i (%s) ..." % (idx+1, len(issues), issue.id)
> @@ -248,9 +343,15 @@
> ET.SubElement(bug, "title").text = issue.summary
> ET.SubElement(bug, "description").text = issue.details
>
> - _sfuser(ET.SubElement(bug, "reporter"), issue.submitter)
> + _mapsfuser(ET.SubElement(bug, "reporter"), issue.submitter)
> + _mapsfuser(ET.SubElement(bug, "assignee"), issue.assignee)
>
> - ET.SubElement(bug, "status").text = STATUS_MAP[issue.status]
> + c = ET.SubElement(bug, "status")
> + status = STATUS_MAP[issue.status]
> + try:
> + c.text = status[issue.resolution]
> + except TypeError:
> + c.text = status

Two remarks: Single characters are deprecated as variable names. What
about "status_node" instead? (also, you use "c" again later in this
function.)

Secondly, I would prefer something like

    if isinstance(status, dict):
        c.text = status[issue.resolution]
    else:
        c.text = status

to the try/except.

[...]
> @@ -308,8 +418,7 @@
> ET.SubElement(at, "mimetype").text = attach.filetype
> ET.SubElement(at, "contents").text = data
>
> -
> - # indent(root)
> + indent(root)

Revision 6 of sfbugs2launchpad disabled this line. The commit message
of this revision is a bit enigmatic ("Disabled pretty printing because
it made problems with extensions"), but it is perhaps better not to
enable it again.