Auto-formatting code with black - object now if you have a strong opinion

[Augie Fackler]

On Wed, Feb 13, 2019 at 10:33:15AM +0100, Boris FELD wrote:
> On 01/12/2018 02:35, Matt Harbison wrote:
> > On Fri, 30 Nov 2018 07:25:04 -0500, Boris FELD
> > <boris.feld at octobus.net> wrote:
> >
> >> I think using automatic formatting is a great idea and we should move
> >> forward with this plan. Black seems a good option. I share other's
> >> concerns about the formatting of import. I also wonder if this also
> >> applies to list and dict formatting that we tend to express with one
> >> value per line for clarity.
> >
> > It looks like yes, unfortunately, if it fits on one line:
> >
> > diff --git a/hgext/lfs/blobstore.py b/hgext/lfs/blobstore.py
> > --- a/hgext/lfs/blobstore.py
> > +++ b/hgext/lfs/blobstore.py
> > @@ -289,50 +289,47 @@ class _gitlfsremote(object):
> >          Return decoded JSON object like {'objects': [{'oid': '',
> > 'size': 1}]}
> >          See
> > https://github.com/git-lfs/git-lfs/blob/master/docs/api/batch.md
> >          """
> > -        objects = [{'oid': p.oid(), 'size': p.size()} for p in pointers]
> > -        requestdata = json.dumps({
> > -            'objects': objects,
> > -            'operation': action,
> > -        })
> > -        url = '%s/objects/batch' % self.baseurl
> > +        objects = [{"oid": p.oid(), "size": p.size()} for p in pointers]
> > +        requestdata = json.dumps({"objects": objects, "operation":
> > action})
> > +        url = "%s/objects/batch" % self.baseurl
> >          batchreq = util.urlreq.request(url, data=requestdata)
> > ...
>
> We have been discussing with the Black author about how we could handle
> those cases and we found a `hack` which is adding an empty comment on
> the first line of a list, dict, multi-line construction:

If you're talking to Łukasz, could you float the way clang-format
works by him? That is, if a trailing comma is present in the literal
it's formatted one element per line, but if there's no trailing comma
it's formatted compactly. I've found that to be a good tradeoff. It'd
also make our import blocks format correctly without modification.

>
> For example:
>
> requestdata = json.dumps({
>     #
>     'objects': objects,
>     'operation': action,
>
> })

I...guess I can live with that. Begrudgingly.

>
> Would get transformed into:
>
> requestdata = json.dumps(
>     {
>         #
>         "objects": objects,
>         "operation": action,
>     }
> )
>
> which is then stable.
>
> > I'm also concerned about stuff like this, which seems far less
> > readable than the original (especially the conditional):
> >
> > diff --git a/hgext/relink.py b/hgext/relink.py
> > --- a/hgext/relink.py
> > +++ b/hgext/relink.py
> > @@ -56,29 +50,32 @@ def relink(ui, repo, origin=None, **opts
> >      command is running. (Both repositories will be locked against
> >      writes.)
> >      """
> > -    if (not util.safehasattr(util, 'samefile') or
> > -        not util.safehasattr(util, 'samedevice')):
> > -        raise error.Abort(_('hardlinks are not supported on this
> > system'))
> > -    src = hg.repository(repo.baseui, ui.expandpath(origin or
> > 'default-relink',
> > -                                          origin or 'default'))
> > -    ui.status(_('relinking %s to %s\n') % (src.store.path,
> > repo.store.path))
> > +    if not util.safehasattr(util, "samefile") or not util.safehasattr(
> > +        util, "samedevice"
> > +    ):
> > +        raise error.Abort(_("hardlinks are not supported on this
> > system"))
> > +    src = hg.repository(
> > +        repo.baseui, ui.expandpath(origin or "default-relink", origin
> > or "default")
> > +    )
> > +    ui.status(_("relinking %s to %s\n") % (src.store.path,
> > repo.store.path))
> >      if repo.root == src.root:
> Black output is not final yet, Black author wants to have the
> possibility to make bugfixes. This particular example might be a bug
> that could be solved. It could also be solved by extracting parameters
> into variables.
> >
> > This was actually in the first file that I randomly sampled.  I think
> > there were a few more instances like this, but don't feel motivated to
> > find them now.  There were a bunch of lines (in lfs anyway) that were
> > flattened out, and were more readable.  But that was before I saw that
> > the default formatting is 88 columns.  So maybe allowing longer lines
> > would help?  (At the cost of possibly rolling up more lists and dicts
> > into a single line.)
> >
> > I'm not adamantly opposed, and the idea of combining version control
> > and tooling to enforce a format is intriguing.  But FWIW, I'm not
> > thrilled with the result of this.
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel at mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel