Code review comment for lp:~wgrant/launchpad/export-archive-dependencies

On August 7, 2009, Celso Providelo wrote:
> > class IArchiveDependency(Interface):
> > """ArchiveDependency interface."""
> > + export_as_webservice_entry()
> >
> > id = Int(title=_("The archive ID."), readonly=True)
> >
> > - date_created = Datetime(
> > - title=_("Instant when the dependency was created."),
> > - required=False, readonly=True)
> > -
> > - archive = Choice(
> > - title=_('Target archive'),
> > - required=True,
> > - vocabulary='PPA',
> > - description=_("The PPA affected by this dependecy."))
> > -
> > - dependency = Object(
> > - schema=IArchive,
> > - title=_("The archive set as a dependency."),
> > - required=False)
> > -
> > - pocket = Choice(
> > - title=_("Pocket"), required=True,
> > vocabulary=PackagePublishingPocket) + date_created = exported(
> > + Datetime(
> > + title=_("Instant when the dependency was created."),
> > + required=False, readonly=True))
> > +
> > + archive = exported(
> > + ReferenceChoice(
> > + title=_('Target archive'),
> > + required=True,
> > + vocabulary='PPA',
> > + schema=IArchive,
> > + description=_("The PPA affected by this dependecy.")))
> > +
>
> This was a previous design slip, not your fault, but you can probably fix
> it.
>
> Any archive purpose can contain dependencies, not only PPAs. The
> restriction can remain in the form interface
> (IArchiveEditDependenciesForm), so you don't need to modify any test.
>
> I believe you can test this by creating a dependency for a COPY
> (rebuild) archive. In the best case, I think ReferenceChoice() is
> doing nothing and could be replaced with a simple Reference() mainly
> for simplicity.
>
> > + dependency = exported(
> > + Reference(
> > + schema=IArchive,
> > + title=_("The archive set as a dependency."),
> > + required=False))
> > +
> > + pocket = exported(
> > + Choice(
> > + title=_("Pocket"), required=True,
> > + vocabulary=PackagePublishingPocket))
> >
> > component = Choice(
> > title=_("Component"), required=True, vocabulary='Component')

Another thing to watch out for here is that most of these attributes are
read/write. Is that really intented?

Shouldn't a IArchiveDependency object be a relatively immutable objects that
are created/destroyed, but not modified?

Does it make sense to change the dependency and archive attribute directly? If
not, you need to put readonly=True on those attributes.

--
Francis J. Lacoste
<email address hidden>