On Tue, Nov 11, 2014 at 07:03:17AM -0500, Jason Pleau wrote:
> Updated patch
Hey Jason, once again thanks a lot for your patch! It looks good in
general. Some comments/requests:
> debsources/app/views.py | 16 +++++++++++++++-
> debsources/migrate/007-to-008.sql | 14 ++++++++++++++
> debsources/models.py | 10 ++++++----
> debsources/tests/test_webapp.py | 10 ++++++++++
> doc/db-schema/debsources.dia | 23 +++++++++++++++++++++++
> doc/db-schema/debsources.dot | 2 +-
> doc/db-schema/debsources.html | 18 +++++++++++++++++-
> doc/db-schema/debsources.neato | 2 +-
> doc/db-schema/debsources.xml | 16 ++++++++++++++++
> doc/db-schema/debsources.zigzag.dia | 23 +++++++++++++++++++++++
Can you please separate your patch into two separate commits: one that
implements your "real" changes, and a subsequent one that only
regenerates the doc/db-schema/* files? That makes both the history more
readable/clear and reviewing easier.
> diff --git a/debsources/migrate/007-to-008.sql b/debsources/migrate/007-to-008.sql
> new file mode 100644
> index 0000000..838346e
> --- /dev/null
> +++ b/debsources/migrate/007-to-008.sql
> @@ -0,0 +1,14 @@
> +ALTER TABLE suites_info
> + ADD COLUMN alias VARCHAR;
Regarding this, which is an important design decision here, I'm not
particularly happy about having a maximum of 1 alias per suite. Why
couldn't a suite have more?
Now, implement this properly in SQL would require a separate table to
allow 1:N suite<->aliases mappings, which is probably a tad overkill for
what we need here. So how about instead: 1/ renaming the new column to
"aliases", and 2/ document that it is a comma-separated list of aliases,
that all map to the current row in the suites_info table?
Of course your changes to views.py and models.py will need to be adapted
accordingly.
> +UPDATE suites_info
> + SET alias='unstable' WHERE name='sid';
> +
> +UPDATE suites_info
> + SET alias='testing' WHERE name='jessie';
> +
> +UPDATE suites_info
> + SET alias='stable' WHERE name='wheezy';
> +
> +UPDATE suites_info
> + SET alias='oldstable' WHERE name='squeeze';
This is another part of the design that bothers me a little. The above
is OK for deploying quickly the change, but I certainly do not want to
introduce another place in Debsources which will have to be manually
maintained/updated at each Debian release.
So what I think we should do here is modifying the Debsources updated to
retrieve aliases from Release files, and re-fill the suites_info table
(including aliases information) at each update run. What do you think?
> @@ -194,15 +194,17 @@ class SuiteInfo(Base):
> version = Column(String, nullable=True)
> release_date = Column(Date, nullable=True)
> sticky = Column(Boolean, nullable=False)
> + alias = Column(String, nullable=True)
If you're OK with my proposed change, it'd be nice to have automatically
split of the aliases list every time a SuiteInfo object is created, but
I'm not sure what's the most appropriate way to do that in SQLAlchemy.
If it's annoying to do, we can certainly live with keeping that as a
comma-separated string and split it when needed.
> + def __init__(self, name, sticky=False, version=None, release_date=None,
> + alias=None):
> self.name = name
> if version:
> self.version = version
> if release_date:
> self.release_date = release_date
> self.sticky = sticky
> -
> + self.alias = alias
Shouldn't this be instead:
if alias: # "aliases", in the future
self.alias = alias
> diff --git a/doc/db-schema/debsources.dia b/doc/db-schema/debsources.dia
> index 5487cb0..960f76e 100644
> --- a/doc/db-schema/debsources.dia
> +++ b/doc/db-schema/debsources.dia
> @@ -3344,6 +3344,29 @@
From now on there is the auto generated stuff which I think should go in
a separate commit.
HTH,
Cheers.
--
Stefano Zacchiroli . . . . . . . zack@upsilon.cc . . . . o . . . o . o
Maître de conférences . . . . . http://upsilon.cc/zack . . . o . . . o o
Former Debian Project Leader . . @zack on identi.ca . . o o o . . . o .
« the first rule of tautology club is the first rule of tautology club »
Attachment:
signature.asc
Description: Digital signature