launchpad-dev team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

On Mon, Jul 25, 2011 at 9:11 PM, Gavin Panella
<gavin.panella@xxxxxxxxxxxxx> wrote:
> 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.

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