launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/code/model/branch.py'
> --- lib/lp/code/model/branch.py	2015-07-08 16:05:11 +0000
> +++ lib/lp/code/model/branch.py	2015-08-05 00:27:42 +0000
> @@ -814,6 +815,10 @@
>          deletion_operations.extend(
>              DeletionCallable.forSourcePackageRecipe(recipe)
>              for recipe in self.recipes)
> +        if not getUtility(ISnapSet).findByBranch(self).is_empty():
> +            alteration_operations.append(DeletionCallable(
> +                None, _('Some snap packages use this branch.'),
> +                getUtility(ISnapSet).detachFromBranch, self))

s/use/build from/, possibly. And also for the Git case.

>          return (alteration_operations, deletion_operations)
>  
>      def deletionRequirements(self):
> 
> === modified file 'lib/lp/snappy/model/snap.py'
> --- lib/lp/snappy/model/snap.py	2015-08-03 13:16:48 +0000
> +++ lib/lp/snappy/model/snap.py	2015-08-05 00:27:42 +0000
> @@ -334,6 +335,25 @@
>          """See `ISnapSet`."""
>          return IStore(Snap).find(Snap, Snap.owner == owner)
>  
> +    def findByBranch(self, branch):
> +        """See `ISnapSet`."""
> +        return IStore(Snap).find(Snap, Snap.branch == branch)
> +
> +    def findByGitRepository(self, repository):
> +        """See `ISnapSet`."""
> +        return IStore(Snap).find(Snap, Snap.git_repository == repository)
> +
> +    def detachFromBranch(self, branch):
> +        """See `ISnapSet`."""
> +        IStore(Snap).execute(Update(
> +            {Snap.branch_id: None}, Snap.branch == branch))
> +
> +    def detachFromGitRepository(self, repository):
> +        """See `ISnapSet`."""
> +        IStore(Snap).execute(Update(
> +            {Snap.git_repository_id: None, Snap.git_path: None},
> +            Snap.git_repository == repository))

The two detach methods can just be self.findByFoo(bar).set(foo=None), I think.

You might also consider whether setting date_last_modified is justified.

> +
>      def empty_list(self):
>          """See `ISnapSet`."""
>          return []


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-source-deletion/+merge/266866
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.