Hello Stefano, Thanks for your feedback on this. I know how time consuming it can be to review someone else's code when they're new to a project, so please know I do appreciate it !
I have a couple of comments / questions to some of your suggestions : On 13/11/14 04:37 PM, Stefano Zacchiroli wrote: > >> 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. > The idea of having multiple aliases per suite never crossed my mind. I'm not sure it exists right now in Debian but that's a good idea. The logical thing would be to create a separate table to allow the association between a suite and its aliases. I think it would ease the rest of the code since we'd let SQLAlchemy handle creating and saving the 'aliases' array. However if you're 100% certain that there will never be a need to link specific data to a suite+alias association (I can't think of any), storing a comma-separated string of aliases in the 'suites' table would be fine for me. >> +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? > Agreed about having the suites and their aliases auto-updated. I remember we talked about it a bit on #debian-qa with pabs, there was the possibility to use an API once it's finished: https://api.ftp-master.debian.org/suites If the API is not close to release, I can look into parsing Release files, are you aware of a parsing tool in python? I was looking at python-apt, but it seems to rely on a sources.list file, I don't think that's not what we need. >> @@ -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. > It would be annoying to have to split the comma-separated string every time we want to make use of the aliases. I vote for splitting it into a list (or tuple?) when a SuiteInfo object is instantiated. (That's if we do it with a string, and not a 1:N mapping table) > HTH, > Cheers. > Thanks -- Jason Pleau -- To UNSUBSCRIBE, email to debian-qa-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: https://lists.debian.org/54653f25.70...@jpleau.ca