Code review comment for lp:~timrchavez/launchpad/set_ppa_private_from_api_724740-2

Hi Tim--

This looks pretty good, I have one comment (regarding the lint issue you pointed out) and one question regarding sourcedeps.cache

> === modified file 'lib/lp/soyuz/model/archive.py'
> --- lib/lp/soyuz/model/archive.py 2011-06-07 05:02:53 +0000
> +++ lib/lp/soyuz/model/archive.py 2011-06-09 03:09:34 +0000
> @@ -259,7 +260,7 @@
>
> publish = BoolCol(dbName='publish', notNull=True, default=True)
>
> - private = BoolCol(dbName='private', notNull=True, default=False,
> + _private = BoolCol(dbName='private', notNull=True, default=False,
> storm_validator=_validate_archive_privacy)
>
> require_virtualized = BoolCol(
> @@ -324,6 +325,19 @@
> alsoProvides(self, IDistributionArchive)
>
> @property
> + def private(self):
> + return self._private
> +
> + @private.setter
> + def private(self, private):
> + self._private = private
> + if private:
> + if not self.buildd_secret:
> + self.buildd_secret = create_token(20)
> + else:
> + self.buildd_secret = Nonek
> +
> + @property
> def title(self):
> """See `IArchive`."""
> return self.displayname

I may be out of date, but the way I've usually seen this done is

+ def _get_private(self):
+ return self._private
+
+ def _set_private(self):
+ self._private = private
+ if private:
+ if not self.buildd_secret:
+ self.buildd_secret = create_token(20)
+ else:
+ self.buildd_secret = None
+ private = property(_get_private, _set_private)

I see that .setter attr was introduced on the @property decorator
in 2.6; it's possible lint wasn't updated to catch this.

As @property.setter works, there's no need to change this, just providing an explanation. It may be worth it to add a comment indicating lint will complain so other people changing this file don't have to figure it out again themselves.

> === modified file 'utilities/sourcedeps.cache'
> --- utilities/sourcedeps.cache 2011-06-01 18:58:15 +0000
> +++ utilities/sourcedeps.cache 2011-06-09 03:09:34 +0000
> @@ -1,4 +1,8 @@
> {
> + "bzr-builder": [
> + 68,
> + "<email address hidden>"
> + ],
> "testresources": [
> 16,
> "<email address hidden>"
> @@ -27,18 +31,14 @@
> 24,
> "<email address hidden>"
> ],
> + "lpreview": [
> + 23,
> + "<email address hidden>"
> + ],
> "bzr-git": [
> 259,
> "<email address hidden>"
> ],
> - "loggerhead": [
> - 445,
> - "<email address hidden>"
> - ],
> - "bzr-builder": [
> - 68,
> - "<email address hidden>"
> - ],
> "bzr-loom": [
> 49,
> "<email address hidden>"
> @@ -47,9 +47,9 @@
> 4,
> "sinzui-20090526164636-1swugzupwvjgomo4"
> ],
> - "lpreview": [
> - 23,
> - "<email address hidden>"
> + "loggerhead": [
> + 445,
> + "<email address hidden>"
> ],
> "difftacular": [
> 6,

I'm not sure what's going on in sourcedeps here. Did a tool
automatically shuffle these, or was there some goal or scheme
you were reorganizing them for?