(+cc people that I saw on this thread with opinions)
> On Nov 30, 2018, at 20:35, Matt Harbison <mharbiso...@gmail.com> wrote:
>
> On Fri, 30 Nov 2018 07:25:04 -0500, Boris FELD <boris.f...@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)
> ...
>
>
> 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:
>
> 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.
Those are...unfortunate. My bias, after years of using source-to-source
formatters, is that I'd rather have machine-enforced known-stable formatting
that's occasionally suboptimal (un-wrapping container literals is annoying and
also a "feature" of clang-format) than have to think about doing my own
formatting (especially wrapping). I know I wouldn't have had this position ~5
years ago, but it's remarkably liberating to not have to worry about formatting.
I think I have an idea for the imports case, but I'll hold off on doing
anything until I feel like there's more consensus on if we should actually try
black or not. I'm also open to ideas on how we could experiment with it in a
contained way. Maybe we could try running it on only hgext/remotefilelog for a
few weeks and see how people feel about it in those files? We'd still have to
do some edits to check-code in order to make that work, but nothing too
complicated...
As far as yapf goes, we use it at Google, and it's not as strong-willed as
black, which cuts both ways. On the positive side, it will leave a container as
spanning many lines as long as you leave a trailing comma after the last entry
(that is, [foo,bar,] -> should be multiline, [foo, bar] -> should be single
line), but on the negative last I checked it wasn't as deterministic, and
depending on the formatting that went in it could produce different formatting
on the way out. That means you're somewhat more likely to end up with spurious
formatting diffs because you perturb some whitespace. black takes a much more
draconian approach where for a given AST stream it wants to produce the same
output file, with no concern given to the initial whitespace. I'd be happy to
try out yapf too, if there's sufficient interest: I think black is better
overall, but yapf would be better than continuing to manually format things.
So, options to move forward:
1) blacken everything (controversial for good reasons)
2) try black only on a subset
3) explore yapf
4) Give up and keep manually formatting files (I'd rather not do this, but I
understand if it's where we end up)
I'm willing to do some of the work on this, but I'd like to know where I should
invest time before I do anything. Any opinions what is likely to be an
efficient use of my time?
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
