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. [...] > 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. 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. But this could be beneficial for an external contributor. However, if they tidy a file that is mostly not their code they'll create a big noisy diff... and then we'd be better off just tidying everything all the time. If PythonTidy can be trained to have a very light touch then I think it would be worth it. By that I mean only reformatting code that would otherwise cause lint warnings, or those that are egregiously wrong (but that itself is difficult to quantify). I don't think we have enough of a problem to warrant something more heavy-handed. 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.) [2] - distro_series = store.find( - DistroSeries, - DistroSeries.distribution == self.distribution, - SourcePackagePublishingHistory.distroseries == DistroSeries.id, - SourcePackagePublishingHistory.archive == self, - SourcePackagePublishingHistory.status.is_in( - active_publishing_status)) + distro_series = store.find(DistroSeries, DistroSeries.distribution + == self.distribution, + SourcePackagePublishingHistory.distroseries + == DistroSeries.id, + SourcePackagePublishingHistory.archive + == self, + SourcePackagePublishingHistory.status.is_in(active_publishing_status)) This is an especially bad example of [1]. I think most of the problems with the patch are bad examples of [1]. [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. [4] - dependencies = ArchiveDependency.select( - query, clauseTables=clauseTables, orderBy=orderBy) + dependencies = ArchiveDependency.select(query, + clauseTables=clauseTables, orderBy=orderBy) Imo this is wrong. [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). [6] - clauses = [""" + + clauses = \ + [""" Why! [7] - """ % sqlvalues(name)) + """ + % sqlvalues(name)) Why! (I assume that this is a bug, and that [6] is similar.) [8] - clauses, clauseTables, orderBy = self._getBinaryPublishingBaseClauses( - name=name, version=version, status=status, pocket=pocket, - distroarchseries=distroarchseries, exact_match=exact_match) + (clauses, clauseTables, orderBy) = \ + self._getBinaryPublishingBaseClauses( + name=name, + version=version, ... This is inconsistent. In some cases it has tried to pack, or paragraph wrap, arguments - e.g. left-align-with-opening-brace - and here it's putting one per line, without "correct" indentation. [9] - return self.getPublishedOnDiskBinaries( - status=PackagePublishingStatus.PUBLISHED).count() + return self.getPublishedOnDiskBinaries(status=PackagePublishingStatus.PUBLISHED).count() It does this quite often: reformats something that's fine into an overlong line that will now cause lint warnings. _______________________________________________ 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
