Merge lp://staging/~james-w/tarmac/break-up-do-merges into lp://staging/~ubuntuone-hackers/tarmac/trunk

On 18 December 2012 21:26, James Westby <email address hidden> wrote:
> James Westby has proposed merging lp:~james-w/tarmac/break-up-do-merges into lp:~ubuntuone-hackers/tarmac/trunk with lp:~james-w/tarmac/commit-message as a prerequisite.
>
> Requested reviews:
> Ubuntu One hackers (ubuntuone-hackers)
>
> For more details, see:
> https://code.launchpad.net/~james-w/tarmac/break-up-do-merges/+merge/140554
>
> Hi,
>
> This branch breaks up the _do_merges method a little, as it was huge.
>

It was!

See inline.

> I tried to keep changes to a minimum while moving the code around, but it
> used "continue" for flow control, so I had to change it a little to
> use return values.
>

...
> === modified file 'tarmac/bin/commands.py'
> --- tarmac/bin/commands.py 2012-09-05 18:08:50 +0000
> +++ tarmac/bin/commands.py 2012-12-18 21:25:42 +0000
> @@ -134,6 +134,194 @@
> help_commands(self.outf)
>
>
> +def _get_mergable_proposals_for_branch(lp_branch, logger, imply_commit_message=False):
> + """Return a list of the mergable proposals for the given branch."""
> + proposals = []
> + for entry in lp_branch.landing_candidates:
> + logger.debug("Considering merge proposal: {0}".format(entry.web_link))
> +
> + if entry.queue_status != u'Approved':
> + logger.debug(
> + " Skipping proposal: status is {0}, not "
> + "'Approved'".format(entry.queue_status))
> + continue
> +
> + if (not imply_commit_message and not entry.commit_message):
> + logger.debug(
> + " Skipping proposal: proposal has no commit message")
> + continue
> +
> + proposals.append(entry)
> + return proposals
> +

This seems a natural candidate for an iterator. Still, no big deal.

> +def _get_reviews(proposal):
> + """Get the set of reviews from the proposal."""
> + reviews = []
> + for vote in proposal.votes:
> + if not vote.comment:
> + continue
> + else:
> + reviews.append('%s;%s' % (vote.reviewer.display_name,
> + vote.comment.vote))
> +
> + if len(reviews) == 0:
> + return None
> +
> + return reviews
> +

Again, a potential iterator, and again, I don't care enough for this branch.

Implementation could probably be more functional, but even this is a
huge step forwards.

> +
> +def setup(logger, debug=False, http_debug=False):

"setup" is a noun, "set up" a verb phrase. This should be spelled
"set_up". Also, it should probably say what it's going to set up.

> +def merge_proposals(target, proposals, logger, config, command):
> + logger.debug('Firing tarmac_pre_merge hook')
> + tarmac_hooks.fire('tarmac_pre_merge',
> + command, target)
> +
> + statuses = []
> + try:
> + for proposal in proposals:
> + status = merge_proposal(target, proposal, logger,
> + config, command)
> + statuses.append(status)
> + # This except is here because we need the else and can't have it
> + # without an except as well.

Wat.

try:
  thing()
except:
  raise
else:
  another_thing()
...