Code review comment for lp:~al-maisan/launchpad/disabled-copyarch-519665

Revision history for this message
Gavin Panella (allenap) wrote :

Looks great :)

My only comment is about the following function you use in the test:

> def build_not_pending_and_suspended(build):
> """True if the given build is not pending and suspended."""
> return (
> build.buildstate != BuildStatus.NEEDSBUILD or
> build.buildqueue_record.job.status != JobStatus.SUSPENDED)

It absolutely does not need to change, but consider writing it like
you described it:

  def build_not_pending_and_suspended(build):
      """True if the given build is not pending and suspended."""
      return not (
          build.buildstate == BuildStatus.NEEDSBUILD and
          build.buildqueue_record.job.status == JobStatus.SUSPENDED)

It just took me a few extra mental leaps - which meant a few minutes
in my current state :) - to verify that the description matched the
implementation.

review: Approve

« Back to merge proposal