Hi Jeroen!

Thanks for this review! Very useful comments. I reply inline.

* Jeroen T. Vermeulen <email address hidden> [2010-07-07 11:10:59 -0000]:

> Review: Needs Information code
> Nice MP, nice branch!  Very thorough and clean.  I've been seeing too many MPs without mention of design, caveats, QA, lint etc. lately and this one just has it all—plus a good description of what the branch is supposed to do.
> 
> Of course I have some notes:
> 
> 
> > === modified file 'lib/devscripts/autoland.py'
> > --- lib/devscripts/autoland.py	2010-03-05 21:47:29 +0000
> > +++ lib/devscripts/autoland.py	2010-07-06 06:19:30 +0000
> > @@ -12,6 +12,16 @@
> >      """Raised when we try to get a review message without enough reviewers."""
> >  
> >  
> > +class MissingBugsError(Exception):
> > +    """Raised when we try to land a mp without 'no-qa' tag and no linked
> > +    bugs."""
> > +
> > +
> > +class MissingBugsIncrError(Exception):
> > +    """Raised when we try to land a mp with the 'incr' tag and no linked
> > +    bugs."""
> 
> I don't think it's necessary to say "Raised when."  The derivation from Exception and the name it clear already.
> 
> You could just say "Merge proposal has no linked bugs and no 'no-qa' tag," and "Merge proposal has the 'incr' tag but no linked bugs."  That also gets around the ugly line breaks and the abbreviation "mp."

You're right, I've changed as suggested.

> 
> 
> >  class LaunchpadBranchLander:
> >  
> >      name = 'launchpad-branch-lander'
> > @@ -152,21 +162,49 @@
> >          # URL. Do it ourselves.
> >          return URI(scheme='bzr+ssh', host=host, path='/' + branch.unique_name)
> >  
> > -    def get_commit_message(self, commit_text, testfix=False):
> > +    def get_commit_message(self, commit_text, testfix=False, no_qa=False,
> > +            incr=False):
> 
> That last parameter should be indented to align with the "self" parameter.  We do this differently for method definitions than for method invocations.

Done.

> 
> 
> >          """Get the Launchpad-style commit message for a merge proposal."""
> >          reviews = self.get_reviews()
> >          bugs = self.get_bugs()
> > +        no_qa, incr = check_qa_clauses(bugs, no_qa, incr)
> > +
> >          if testfix:
> >              testfix = '[testfix]'
> >          else:
> >              testfix = ''
> > -        return '%s%s%s %s' % (
> > +
> > +        return '%s%s%s%s%s %s' % (
> >              testfix,
> >              get_reviewer_clause(reviews),
> >              get_bugs_clause(bugs),
> > +            no_qa,
> > +            incr,
> >              commit_text)
> >  
> >  
> > +def check_qa_clauses(bugs, no_qa=False, incr=False):
> > +    """Check the no-qa and incr clauses."""
> 
> Good instinct to separate bits out like this so that they become unit-testable.  This is what made me instantly like this branch.  One bit that bothers me slightly is that it does two related but different things: check that the two options are appropriate, and compose a commit string fragment for them.
> 
> You also end up with get_commit_message doing different things: gather data, delegate the QA-related checking & composing to check_qa_clauses, and composing the rest of the commit string.  AIUI it's the data gathering that makes unit-testing hard, and you did the right thing in not letting it sneak into your own function.
> 
> A rule of thumb I have for maintainable code is that a function should not mix abstractions.  In get_commit_message we have both "fiddle with strings" and "delegate one area of the checking and string-fiddling."  That combination makes it hard to follow (and maintain) the string-fiddling.
> 
> If I can make a few suggestions, it may be worth playing with these ideas:
> 
>  * Stay out of string manipulation for as long as you can.  Try creating a list of tags.  Append any relevant tags, then join.  No more "else: tag = ''" and no more need to get the number of %s right.

Right. 

I've created another method to check the testfix tag, so I removed this bit of string handling from inside get_commit_message. Also, in the get_qa_clause (old check_qa_clauses, renamed for sake of consistency with the other get_foo_clause methods) I return the qa tag, that can be or '[no-qa]', or '[incr]', or empty. To keep things consistent, all get_foo_clause methods return a string that's the clause, and inside get_commit_message I put all of them together on a clause list, and just join them to return the clauses, as suggested. Much better, it seems.

> 
>  * Inline the "if incr: tags.append('[incr]')" into get_commit_message, and similar for no_qa.

I applied the idea.

> 
>  * If only get_bugs and get_reviews use launchpadlib, try writing tests that replace these methods with FakeMethods (lp.testing.fakemethod) that return the values you need for your test.  Now you can unit-test get_commit_message to your heart's content.

Uhh, that's cool! I didn't know about these FakeMethods. I've added
tests to get_commit_message using FakeMethods. I'll write some more when
applying the next changes to this script to implement the continuous
merge workflow.

> 
> It may not be worth it for this script.  But if you have time, it could be a nice refactoring exercise.

:)

> 
> 
> > +
> > +    if not bugs and not no_qa and not incr:
> > +        raise MissingBugsError("Need bugs linked or --no-qa option.")
> 
> If this exception is always caught and replaced with a BzrCommandError with its own error string, it's probably better to leave out this redundant message.
> 
> 
> > +    if incr and not bugs:
> > +        raise MissingBugsIncrError("--incr option requires bugs linked to "
> > +            "the branch.")
> 
> Same here.

That's right, done and done.

> 
> 
> > === modified file 'lib/devscripts/ec2test/builtins.py'
> > --- lib/devscripts/ec2test/builtins.py	2010-04-13 19:32:04 +0000
> > +++ lib/devscripts/ec2test/builtins.py	2010-07-06 06:19:30 +0000
> > @@ -323,6 +323,12 @@
> >              'testfix',
> >              help="Include the [testfix] prefix in the commit message."),
> >          Option(
> > +            'no-qa',
> > +            help="Include the [no-qa] prefix in the commit message."),
> > +        Option(
> > +            'incr',
> > +            help="Include the [incr] prefix in the commit message."),
> 
> Clearly not the fault of your branch, but these help strings assume that we think in terms of these tags and write them all the time.  The more we use this script, the less true that becomes!
> 
> If it's not too much trouble, could you convert these to say things like "This is a testfix (tags commit with [testfix])" and "Does not require QA (tags commit with [no-qa])" and "Incremental to other bug fix (tags commit with [incr])"?
> 
> May affect tests, of course.

That makes perfect sense. I'm changing them accordingly. I also changed
the option from --incr to --incremental, because it's clearer this way.
I kept the tag that should go in the commit message as the short
version, [incr], because I think [incremental] would be too long and this will only be used by internal scripts.

> 
> 
> > @@ -396,12 +407,23 @@
> >                  "Commit text not specified. Use --commit-text, or specify a "
> >                  "message on the merge proposal.")
> >          try:
> > -            commit_message = mp.get_commit_message(commit_text, testfix)
> > +            commit_message = mp.get_commit_message(commit_text, testfix,
> > +                no_qa, incr)
> 
> The Launchpad style lets you move all arguments to the second line:
> 
>             commit_message = mp.get_commit_message(
>                 commit_text, testfix, no_qa, incr)
> 
> 
> >          except MissingReviewError:
> >              raise BzrCommandError(
> >                  "Cannot land branches that haven't got approved code "
> >                  "reviews. Get an 'Approved' vote so we can fill in the "
> >                  "[r=REVIEWER] section.")
> > +        except MissingBugsError:
> > +            raise BzrCommandError(
> > +                "Branch doesn't have linked bugs and doesn't have no-qa "
> > +                "option set. Use --no-qa, or link the related bugs to the "
> > +                "branch.")
> > +        except MissingBugsIncrError:
> > +            raise BzrCommandError(
> > +                "--incr option requires bugs to be linked to the branch. "
> > +                "Link the bugs or remove the --incr option.")
> 
> This is the part where I assume you're catching all instances of these two types of exception, and so can raise the original exceptions without messages.
> 
> 
> > === modified file 'lib/devscripts/tests/test_autoland.py'
> > --- lib/devscripts/tests/test_autoland.py	2009-10-08 23:22:16 +0000
> > +++ lib/devscripts/tests/test_autoland.py	2010-07-06 06:19:30 +0000
> > @@ -11,7 +11,8 @@
> >  
> >  from devscripts.autoland import (
> >      get_bazaar_host, get_bugs_clause, get_reviewer_clause,
> > -    get_reviewer_handle, MissingReviewError)
> > +    get_reviewer_handle, check_qa_clauses, MissingReviewError,
> > +    MissingBugsError, MissingBugsIncrError)
> >  
> >  
> >  class FakeBug:
> > @@ -68,6 +69,52 @@
> >          self.assertEqual('[bug=20,45]', bugs_clause)
> >  
> >  
> > +class TestCheckQaClauses(unittest.TestCase):
> > +    """Tests for `check_qa_clauses`"""
> 
> Very nice, thorough testing here.  And no layer, which means it's fast.
> 
> With the small changes I listed, this branch will be good to land.  I'll leave it up to you whether you want to attempt the larger cleanups.  I'm voting Needs Information for procedural reasons: if you have time for the cleanups I can review them here, or if you don't, let me know and I (or someone else) can approve the branch right away.

Thanks for the detailed review Jeroen, really liked it!

> 
> 
> Jeroen
> -- 
> https://code.launchpad.net/~ursinha/launchpad/add-ec2land-rules-orphaned-branches/+merge/29255
> You are the owner of lp:~ursinha/launchpad/add-ec2land-rules-orphaned-branches.

-- 
Ursula Junque (Ursinha)
<email address hidden>