On Mon, Jul 25, 2011 at 9:11 PM, Gavin Panella <gavin.pane...@canonical.com> wrote: > On 25 July 2011 05:36, Robert Collins <robe...@robertcollins.net> wrote: > [...] >> And we probably want to: >> - add Gavin's import-formatter rules (I think they are nicer because >> of their merge properties( > > For the record, Henning wrote the import formatter.
Doh! Sorry Henning! > [...] >> I've attached the diff that my slightly tweaked tidy script generated >> (and the tweaked script). > > Apart from the problems with imports and not splitting string > literals, I've noted at the end some things I've noticed from the > diff. Thanks; I think most of these do make the code less pleasant, so should be added to the list of things we'd want addressed. > In all I think it makes code less readable. I think that's because the > core team have high standards already and have all read the style > guide. I'm skeptical that any tool will ever match up to that. I don't think a tool needs to match up to that level: we could accept a (not much) worse standard that is consistently applied with no effort. Right now, i think the tool is well below our standard, but probably not hard to bring up to scratch. > (but that itself is difficult to quantify). I don't think we have > enough of a problem to warrant something more heavy-handed. That gets into a tool that understands diffs and regions that changed. or one that is heavy handed but matches our rules ;) The reason being that the tool is a compiler - it compiles the input into the output. > My observations of tidied.patch: > > [1] > > - distribution = ForeignKey( > - foreignKey='Distribution', dbName='distribution', notNull=False) > + distribution = ForeignKey(foreignKey='Distribution', > dbName='distribution' > + , notNull=False) > > The comma is weird. More significantly, at what point does it stop > trying to mush arguments against the right column? I think the > original syntax is better here. (I think left-align-with-opening-brace > is almost but not quite always ugly.) so we're often in violation of PEP8 here. Yes the comma sucks and is a bug. ... > This is an especially bad example of [1]. I think most of the problems > with the patch are bad examples of [1]. I think so too. > [3] > > - db_pubconf = getUtility( > - IPublisherConfigSet).getByDistribution(self.distribution) > + db_pubconf = \ > + > getUtility(IPublisherConfigSet).getByDistribution(self.distribution) > > I think this is one of those times when \ improves readability. :) \o/ > [4] > > - dependencies = ArchiveDependency.select( > - query, clauseTables=clauseTables, orderBy=orderBy) > + dependencies = ArchiveDependency.select(query, > + clauseTables=clauseTables, orderBy=orderBy) > > Imo this is wrong. The 8 space indent is PEP8 ok (see under hanging indents), but not needed (because its a function call not a definition). This could be tweaked too, I think. > [5] > > - def getBuildRecords(self, build_state=None, name=None, pocket=None, > - arch_tag=None, user=None, binary_only=True): > + def getBuildRecords( > + self, > + build_state=None, > + name=None, > + pocket=None, > + arch_tag=None, > + user=None, > + binary_only=True, > + ): > > Not sure about this. Maybe like the new formatting rules this one will > grow on me (and it is merge friendly). yeah. I'm torn on this one. > [6] > > - clauses = [""" > + > + clauses = \ > + [""" > > Why! This is probably due to how it handles docstrings, but I'm sure its fixable. > [7] > > - """ % sqlvalues(name)) > + """ > + % sqlvalues(name)) > > Why! (I assume that this is a bug, and that [6] is similar.) Yeah, i think so. ... > It does this quite often: reformats something that's fine into an > overlong line that will now cause lint warnings. Thanks for so thoroughly picking this apart. -Rob _______________________________________________ Mailing list: https://launchpad.net/~launchpad-dev Post to : launchpad-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
