launchpad-dev team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

On 25 July 2011 05:36, Robert Collins <robertc@xxxxxxxxxxxxxxxxx> 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.