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

login
register
mail settings
Submitter Matt Harbison
Date Dec. 1, 2018, 1:35 a.m.
Message ID <op.ztblcu2g9lwrgf@envy>
Download mbox | patch
Permalink /patch/36886/
State Not Applicable
Headers show

Comments

Matt Harbison - Dec. 1, 2018, 1:35 a.m.
On Fri, 30 Nov 2018 07:25:04 -0500, Boris FELD <boris.feld@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:

Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Augie Fackler - Dec. 4, 2018, 3:06 p.m.
(+cc people that I saw on this thread with opinions)

> On Nov 30, 2018, at 20:35, Matt Harbison <mharbison72@gmail.com> wrote:
> 
> On Fri, 30 Nov 2018 07:25:04 -0500, Boris FELD <boris.feld@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?
Yuya Nishihara - Dec. 5, 2018, 1:23 p.m.
On Tue, 4 Dec 2018 10:06:24 -0500, Augie Fackler wrote:
> 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)

My vote: 3 > 4 > 2 > 1

I'm not super enthusiastic about 100%-machine-forced formatting. I like
consistency level provided by e.g. astyle command. clang-format is pretty
good IMHO, but the black seems to sacrifice the code readability.
Matt Harbison - Dec. 7, 2018, 4:21 a.m.
On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <yuya@tcha.org> wrote:

> On Tue, 4 Dec 2018 10:06:24 -0500, Augie Fackler wrote:
>>
>> 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)
>
> My vote: 3 > 4 > 2 > 1
>
> I'm not super enthusiastic about 100%-machine-forced formatting. I like
> consistency level provided by e.g. astyle command. clang-format is pretty
> good IMHO, but the black seems to sacrifice the code readability.

+1.

That said, I got used to longnamesthataresmooshedtogether, so I can  
probably adjust to anything after awhile.
Augie Fackler - Jan. 9, 2019, 8:30 p.m.
> On Dec 6, 2018, at 23:21, Matt Harbison <mharbison72@gmail.com> wrote:
> 
> On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> On Tue, 4 Dec 2018 10:06:24 -0500, Augie Fackler wrote:
>>> 
>>> 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)
>> 
>> My vote: 3 > 4 > 2 > 1
>> 
>> I'm not super enthusiastic about 100%-machine-forced formatting. I like
>> consistency level provided by e.g. astyle command. clang-format is pretty
>> good IMHO, but the black seems to sacrifice the code readability.
> 
> +1.
> 
> That said, I got used to longnamesthataresmooshedtogether, so I can probably adjust to anything after awhile.

I think I'd still prefer black overall (yapf is less opinionated and requires me to think more), but here's a yapf RFC: https://phab.mercurial-scm.org/D5539

This would at least help _most_ cases of line-too-long, which I think would be good. What do people think?
Matt Harbison - Jan. 10, 2019, 5:14 a.m.
On Wed, 09 Jan 2019 15:30:19 -0500, Augie Fackler <raf@durin42.com> wrote:

>
>
>> On Dec 6, 2018, at 23:21, Matt Harbison <mharbison72@gmail.com> wrote:
>>
>> On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <yuya@tcha.org>  
>> wrote:
>>
>>> On Tue, 4 Dec 2018 10:06:24 -0500, Augie Fackler wrote:
>>>>
>>>> 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)
>>>
>>> My vote: 3 > 4 > 2 > 1
>>>
>>> I'm not super enthusiastic about 100%-machine-forced formatting. I like
>>> consistency level provided by e.g. astyle command. clang-format is  
>>> pretty
>>> good IMHO, but the black seems to sacrifice the code readability.
>>
>> +1.
>>
>> That said, I got used to longnamesthataresmooshedtogether, so I can  
>> probably adjust to anything after awhile.
>
> I think I'd still prefer black overall (yapf is less opinionated and  
> requires me to think more), but here's a yapf RFC:  
> https://phab.mercurial-scm.org/D5539
>
> This would at least help _most_ cases of line-too-long, which I think  
> would be good. What do people think?

I think there's less that I dislike with yapf, but I'm not adamant about  
it.

Since I've never used auto formatting, I'm assuming the general procedure  
is to:

   1) code something the approximates the style
   2) run fix
   3) submit

If that's true, what's there to think about?
Augie Fackler - Jan. 10, 2019, 6:51 p.m.
> On Jan 10, 2019, at 00:14, Matt Harbison <mharbison72@gmail.com> wrote:
> 
> On Wed, 09 Jan 2019 15:30:19 -0500, Augie Fackler <raf@durin42.com> wrote:
> 
>> 
>> 
>>> On Dec 6, 2018, at 23:21, Matt Harbison <mharbison72@gmail.com> wrote:
>>> 
>>> On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
>>> 
>>>> On Tue, 4 Dec 2018 10:06:24 -0500, Augie Fackler wrote:
>>>>> 
>>>>> 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)
>>>> 
>>>> My vote: 3 > 4 > 2 > 1
>>>> 
>>>> I'm not super enthusiastic about 100%-machine-forced formatting. I like
>>>> consistency level provided by e.g. astyle command. clang-format is pretty
>>>> good IMHO, but the black seems to sacrifice the code readability.
>>> 
>>> +1.
>>> 
>>> That said, I got used to longnamesthataresmooshedtogether, so I can probably adjust to anything after awhile.
>> 
>> I think I'd still prefer black overall (yapf is less opinionated and requires me to think more), but here's a yapf RFC: https://phab.mercurial-scm.org/D5539
>> 
>> This would at least help _most_ cases of line-too-long, which I think would be good. What do people think?
> 
> I think there's less that I dislike with yapf, but I'm not adamant about it.

Should I pursue fixing any bugs in yapf, or is this a lukewarm "I'd rather we not bother" sort of feeling?

(if others have opinions I'd like to hear those too)

> 
> Since I've never used auto formatting, I'm assuming the general procedure is to:
> 
>  1) code something the approximates the style

Once I'm on a formatter, I typically write "literally anything that's syntactically valid" and then an editor save hook fixes the formatting to be pretty/correct. yapf isn't picky enough to make that workflow quite reliable, but in practice it's probably fine...

>  2) run fix
>  3) submit
> 
> If that's true, what's there to think about?
Matt Harbison - Jan. 11, 2019, 5:37 a.m.
On Thu, 10 Jan 2019 13:51:12 -0500, Augie Fackler <raf@durin42.com> wrote:

>
>
>> On Jan 10, 2019, at 00:14, Matt Harbison <mharbison72@gmail.com> wrote:
>>
>> On Wed, 09 Jan 2019 15:30:19 -0500, Augie Fackler <raf@durin42.com>  
>> wrote:
>>
>>>
>>>
>>>> On Dec 6, 2018, at 23:21, Matt Harbison <mharbison72@gmail.com> wrote:
>>>>
>>>> On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <yuya@tcha.org>  
>>>> wrote:
>>>>
>>>>> On Tue, 4 Dec 2018 10:06:24 -0500, Augie Fackler wrote:
>>>>>>
>>>>>> 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)
>>>>>
>>>>> My vote: 3 > 4 > 2 > 1
>>>>>
>>>>> I'm not super enthusiastic about 100%-machine-forced formatting. I  
>>>>> like
>>>>> consistency level provided by e.g. astyle command. clang-format is  
>>>>> pretty
>>>>> good IMHO, but the black seems to sacrifice the code readability.
>>>>
>>>> +1.
>>>>
>>>> That said, I got used to longnamesthataresmooshedtogether, so I can  
>>>> probably adjust to anything after awhile.
>>>
>>> I think I'd still prefer black overall (yapf is less opinionated and  
>>> requires me to think more), but here's a yapf RFC:  
>>> https://phab.mercurial-scm.org/D5539
>>>
>>> This would at least help _most_ cases of line-too-long, which I think  
>>> would be good. What do people think?
>>
>> I think there's less that I dislike with yapf, but I'm not adamant  
>> about it.
>
> Should I pursue fixing any bugs in yapf, or is this a lukewarm "I'd  
> rather we not bother" sort of feeling?

It's lukewarm.  I probably focus too much on what I don't like, knowing it  
can't be fixed.  But in fairness, I think it made some things better,  
which means human formatting isn't great either.

I'm not sure how much time it would require to pursue fixing yapf.  If  
it's not a lot, maybe it's a reasonable middle ground between nothing and  
black?  I like the idea in theory, but don't look forward to retraining my  
mind to read some of these things.  But don't let me dump all over it-  
like I said, I've never seriously tried to use one.  Maybe it won't take  
that long to get used to.
Yuya Nishihara - Jan. 12, 2019, 7:47 a.m.
On Wed, 9 Jan 2019 15:30:19 -0500, Augie Fackler wrote:
> 
> 
> > On Dec 6, 2018, at 23:21, Matt Harbison <mharbison72@gmail.com> wrote:
> > 
> > On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
> > 
> >> On Tue, 4 Dec 2018 10:06:24 -0500, Augie Fackler wrote:
> >>> 
> >>> 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)
> >> 
> >> My vote: 3 > 4 > 2 > 1
> >> 
> >> I'm not super enthusiastic about 100%-machine-forced formatting. I like
> >> consistency level provided by e.g. astyle command. clang-format is pretty
> >> good IMHO, but the black seems to sacrifice the code readability.
> > 
> > +1.
> > 
> > That said, I got used to longnamesthataresmooshedtogether, so I can probably adjust to anything after awhile.
> 
> I think I'd still prefer black overall (yapf is less opinionated and requires me to think more), but here's a yapf RFC: https://phab.mercurial-scm.org/D5539

Thanks for trying it. Maybe I have to agree that the black is less bad.

Why does yapf split tuples unnecessarily?
https://phab.mercurial-scm.org/D5539#C29949NL27

Maybe yapf could be configured to get saner results, but I also heard yapf
configuration is abstract, and it isn't easy to get expected result overall.
Gregory Szorc - Feb. 11, 2019, 6:21 p.m.
I'm reviving this thread because I want us to commit to something.

I *really* want us to mass reformat the repo, mainly so we can make the
source code Python 3 native (so we can do away with our custom source
transforming module importer) ASAP.

I'm not sure if it has been proposed yet, but have we considered vendoring
black and changing its behavior to satisfy our needs? This will also make
it easier for Mercurial developers to incorporate black into workflows:
we'll be able to commit hooks/extensions for running/testing source
formatting without having to worry about 3rd party dependencies being
installed. I don't see a long-term maintainability issue with vendoring
black because whatever version of black we vendor should continue to run
for the foreseeable future: the only thing I can think of that would cause
it to break would be the introduction of new symbols in Python's grammar,
and that tends to be rare. And even when it does, Mercurial won't adopt
them for years due to BC requirements.

On Fri, Jan 11, 2019 at 11:50 PM Yuya Nishihara <yuya@tcha.org> wrote:

> On Wed, 9 Jan 2019 15:30:19 -0500, Augie Fackler wrote:
> >
> >
> > > On Dec 6, 2018, at 23:21, Matt Harbison <mharbison72@gmail.com> wrote:
> > >
> > > On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <yuya@tcha.org>
> wrote:
> > >
> > >> On Tue, 4 Dec 2018 10:06:24 -0500, Augie Fackler wrote:
> > >>>
> > >>> 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)
> > >>
> > >> My vote: 3 > 4 > 2 > 1
> > >>
> > >> I'm not super enthusiastic about 100%-machine-forced formatting. I
> like
> > >> consistency level provided by e.g. astyle command. clang-format is
> pretty
> > >> good IMHO, but the black seems to sacrifice the code readability.
> > >
> > > +1.
> > >
> > > That said, I got used to longnamesthataresmooshedtogether, so I can
> probably adjust to anything after awhile.
> >
> > I think I'd still prefer black overall (yapf is less opinionated and
> requires me to think more), but here's a yapf RFC:
> https://phab.mercurial-scm.org/D5539
>
> Thanks for trying it. Maybe I have to agree that the black is less bad.
>
> Why does yapf split tuples unnecessarily?
> https://phab.mercurial-scm.org/D5539#C29949NL27
>
> Maybe yapf could be configured to get saner results, but I also heard yapf
> configuration is abstract, and it isn't easy to get expected result
> overall.
>
Augie Fackler - Feb. 11, 2019, 6:56 p.m.
> On Feb 11, 2019, at 13:21, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> 
> I'm reviving this thread because I want us to commit to something.
> 
> I *really* want us to mass reformat the repo, mainly so we can make the source code Python 3 native (so we can do away with our custom source transforming module importer) ASAP.

Maybe take a look at the output yapf has with `split_all_comma_separated_values = true` removed from the config (see https://phab.mercurial-scm.org/D5539 for a starting point, alter the config and then run `hg status --rev .^ -m 'set:**/*.py' -n | xargs yapf -i` to see what changes) - that looks like it might be close enough we could live with it for at least until PyCon - I'd be happy to try and snag ambv at PyCon (I assume he'll be there) and see if I can convince him I'm right enough he should consider a patch.

> 
> I'm not sure if it has been proposed yet, but have we considered vendoring black and changing its behavior to satisfy our needs?

The only major problem with black is un-wrapping dict/set/tuple/list literals that we'd rather keep wrapped, right? I think if https://github.com/ambv/black/issues/601 was resolved in a manner similar to clang-format we'd be able to get everything we want.

> This will also make it easier for Mercurial developers to incorporate black into workflows: we'll be able to commit hooks/extensions for running/testing source formatting without having to worry about 3rd party dependencies being installed. I don't see a long-term maintainability issue with vendoring black because whatever version of black we vendor should continue to run for the foreseeable future: the only thing I can think of that would cause it to break would be the introduction of new symbols in Python's grammar, and that tends to be rare. And even when it does, Mercurial won't adopt them for years due to BC requirements.
> 
> On Fri, Jan 11, 2019 at 11:50 PM Yuya Nishihara <yuya@tcha.org> wrote:
> On Wed, 9 Jan 2019 15:30:19 -0500, Augie Fackler wrote:
> > 
> > 
> > > On Dec 6, 2018, at 23:21, Matt Harbison <mharbison72@gmail.com> wrote:
> > > 
> > > On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
> > > 
> > >> On Tue, 4 Dec 2018 10:06:24 -0500, Augie Fackler wrote:
> > >>> 
> > >>> 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)
> > >> 
> > >> My vote: 3 > 4 > 2 > 1
> > >> 
> > >> I'm not super enthusiastic about 100%-machine-forced formatting. I like
> > >> consistency level provided by e.g. astyle command. clang-format is pretty
> > >> good IMHO, but the black seems to sacrifice the code readability.
> > > 
> > > +1.
> > > 
> > > That said, I got used to longnamesthataresmooshedtogether, so I can probably adjust to anything after awhile.
> > 
> > I think I'd still prefer black overall (yapf is less opinionated and requires me to think more), but here's a yapf RFC: https://phab.mercurial-scm.org/D5539
> 
> Thanks for trying it. Maybe I have to agree that the black is less bad.
> 
> Why does yapf split tuples unnecessarily?
> https://phab.mercurial-scm.org/D5539#C29949NL27
> 
> Maybe yapf could be configured to get saner results, but I also heard yapf
> configuration is abstract, and it isn't easy to get expected result overall.
Boris Feld - Feb. 13, 2019, 9:33 a.m.
On 01/12/2018 02:35, Matt Harbison wrote:
> On Fri, 30 Nov 2018 07:25:04 -0500, Boris FELD
> <boris.feld@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:

For example:

requestdata = json.dumps({
    #
    'objects': objects,
    'operation': action,

})

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@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Augie Fackler - May 8, 2019, 7:42 p.m.
> On Dec 6, 2018, at 23:21, Matt Harbison <mharbison72@gmail.com> wrote:
> 
> On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> On Tue, 4 Dec 2018 10:06:24 -0500, Augie Fackler wrote:
>>> 
>>> 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)
>> 
>> My vote: 3 > 4 > 2 > 1
>> 
>> I'm not super enthusiastic about 100%-machine-forced formatting. I like
>> consistency level provided by e.g. astyle command. clang-format is pretty
>> good IMHO, but the black seems to sacrifice the code readability.
> 
> +1.
> 
> That said, I got used to longnamesthataresmooshedtogether, so I can probably adjust to anything after awhile.

I've gotten frustrated with yapf and explored autopep8 - yapf is frustrating, and autopep8 isn't very good at wrapping lines. Fortunately, at PyCon I was able to convince ambv that trailing-comma-implies-multiline was a good feature, and the code for that is under review.

https://phab.mercurial-scm.org/D6342 is an updated version of the blackening preview with my black patch applied. I'm very happy with the results (compare with the previous round - in particular imports are cleaner.) It's not 100% what we want, specifically:

 * imports of a single name will still get forced down to a single line
 * one-tuples will get forced down to a single line, even if they started on multiple lines

Overall I'm much happier with the results now - I can make another change that blackens more files if people are curious and not opposed to moving forward with black. Once we can reach a consensus on a formatter, we can think about running byteify-strings on everything. Realistically byteifying strings will require us to be on an auto-formatter because of all the too-long lines it will otherwise create.

Thanks,
Augie
via Mercurial-devel - May 8, 2019, 8:53 p.m.
*From: *Augie Fackler <raf@durin42.com>
*Date: *Wed, May 8, 2019, 12:45
*To: *Matt Harbison
*Cc: *mercurial-devel@mercurial-scm.org, Gregory Szorc


>
> > On Dec 6, 2018, at 23:21, Matt Harbison <mharbison72@gmail.com> wrote:
> >
> > On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <yuya@tcha.org>
> wrote:
> >
> >> On Tue, 4 Dec 2018 10:06:24 -0500, Augie Fackler wrote:
> >>>
> >>> 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)
> >>
> >> My vote: 3 > 4 > 2 > 1
> >>
> >> I'm not super enthusiastic about 100%-machine-forced formatting. I like
> >> consistency level provided by e.g. astyle command. clang-format is
> pretty
> >> good IMHO, but the black seems to sacrifice the code readability.
> >
> > +1.
> >
> > That said, I got used to longnamesthataresmooshedtogether, so I can
> probably adjust to anything after awhile.
>
> I've gotten frustrated with yapf and explored autopep8 - yapf is
> frustrating, and autopep8 isn't very good at wrapping lines. Fortunately,
> at PyCon I was able to convince ambv that trailing-comma-implies-multiline
> was a good feature, and the code for that is under review.
>
> https://phab.mercurial-scm.org/D6342 is an updated version of the
> blackening preview with my black patch applied. I'm very happy with the
> results (compare with the previous round - in particular imports are
> cleaner.) It's not 100% what we want, specifically:
>
>  * imports of a single name will still get forced down to a single line
>

That's at least more merge-friendly.

 * one-tuples will get forced down to a single line, even if they started
> on multiple lines
>

I don't know what this means. One-tuples are very rare anyway, so it
probably doesn't matter much.


> Overall I'm much happier with the results now - I can make another change
> that blackens more files if people are curious and not opposed to moving
> forward with black. Once we can reach a consensus on a formatter, we can
> think about running byteify-strings on everything. Realistically byteifying
> strings will require us to be on an auto-formatter because of all the
> too-long lines it will otherwise create.
>
> Thanks,
> Augie
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Augie Fackler - May 8, 2019, 9:05 p.m.
> On May 8, 2019, at 16:53, martinvonz <martinvonz@google.com> wrote:
> 
>>  * one-tuples will get forced down to a single line, even if they started on multiple lines
> 
> I don't know what this means. One-tuples are very rare anyway, so it probably doesn't matter much.

If you write this:

FOO = (
    'bar',
)

black will make it be

FOO = ('bar',)

even though there's a trailing comma, because it's the one place where a trailing comma is required in Python. Does that make sense?

(list/set/dict literals won't be afflicted in this way, just tuples.)
via Mercurial-devel - May 8, 2019, 9:13 p.m.
*From: *Augie Fackler <raf@durin42.com>
*Date: *Wed, May 8, 2019 at 2:05 PM
*To: *martinvonz
*Cc: *Matt Harbison, Mercurial-devel, Gregory Szorc


>
> > On May 8, 2019, at 16:53, martinvonz <martinvonz@google.com> wrote:
> >
> >>  * one-tuples will get forced down to a single line, even if they
> started on multiple lines
> >
> > I don't know what this means. One-tuples are very rare anyway, so it
> probably doesn't matter much.
>
> If you write this:
>
> FOO = (
>     'bar',
> )
>
> black will make it be
>
> FOO = ('bar',)
>
> even though there's a trailing comma, because it's the one place where a
> trailing comma is required in Python. Does that make sense?
>

The example makes sense, but why would we want a one-tuple on multiple
lines?


>
> (list/set/dict literals won't be afflicted in this way, just tuples.)
Augie Fackler - May 8, 2019, 9:21 p.m.
> On May 8, 2019, at 17:13, Martin von Zweigbergk <martinvonz@google.com> wrote:
> 
> The example makes sense, but why would we want a one-tuple on multiple lines?

for the same reason we'd want a one-element list split that way: avoiding merge conflicts on future expansion. It's rare, but it's a known caveat so I mentioned it.
Gregory Szorc - May 9, 2019, 2:28 a.m.
On Wed, May 8, 2019 at 12:42 PM Augie Fackler <raf@durin42.com> wrote:

>
>
> > On Dec 6, 2018, at 23:21, Matt Harbison <mharbison72@gmail.com> wrote:
> >
> > On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <yuya@tcha.org>
> wrote:
> >
> >> On Tue, 4 Dec 2018 10:06:24 -0500, Augie Fackler wrote:
> >>>
> >>> 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)
> >>
> >> My vote: 3 > 4 > 2 > 1
> >>
> >> I'm not super enthusiastic about 100%-machine-forced formatting. I like
> >> consistency level provided by e.g. astyle command. clang-format is
> pretty
> >> good IMHO, but the black seems to sacrifice the code readability.
> >
> > +1.
> >
> > That said, I got used to longnamesthataresmooshedtogether, so I can
> probably adjust to anything after awhile.
>
> I've gotten frustrated with yapf and explored autopep8 - yapf is
> frustrating, and autopep8 isn't very good at wrapping lines. Fortunately,
> at PyCon I was able to convince ambv that trailing-comma-implies-multiline
> was a good feature, and the code for that is under review.
>
> https://phab.mercurial-scm.org/D6342 is an updated version of the
> blackening preview with my black patch applied. I'm very happy with the
> results (compare with the previous round - in particular imports are
> cleaner.) It's not 100% what we want, specifically:
>
>  * imports of a single name will still get forced down to a single line
>  * one-tuples will get forced down to a single line, even if they started
> on multiple lines
>
> Overall I'm much happier with the results now - I can make another change
> that blackens more files if people are curious and not opposed to moving
> forward with black. Once we can reach a consensus on a formatter, we can
> think about running byteify-strings on everything. Realistically byteifying
> strings will require us to be on an auto-formatter because of all the
> too-long lines it will otherwise create.
>

I'm pretty happy with the state of D6342.

Out of curiosity, is Black opinionated about single versus double quotes?
AFAICT it seems to preserve whatever is there? That somewhat surprises me.

Also, I've mentioned this in other threads but I think it needs to be
repeated here: I view making the source code Python 3 native (read: no
source transforming module importer) as a prerequisite to removing the
"beta" label from Python 3 support. This is because the source transforming
is making some packaging problems very difficult. Because we need to use
bytes literals, I think it is easier for us to adopt a source formatter and
reformat the repo than to do everything by hand. So, the sooner we can get
a style formatter in place, the sooner we can make meaningful progress
towards making the source code Python 3 native and making Mercurial stable
on Python 3.
Matt Harbison - May 9, 2019, 2:55 a.m.
On Wed, 08 May 2019 17:13:30 -0400, Martin von Zweigbergk  
<martinvonz@google.com> wrote:

> *From: *Augie Fackler <raf@durin42.com>
> *Date: *Wed, May 8, 2019 at 2:05 PM
> *To: *martinvonz
> *Cc: *Matt Harbison, Mercurial-devel, Gregory Szorc
>
>
>>
>> > On May 8, 2019, at 16:53, martinvonz <martinvonz@google.com> wrote:
>> >
>> >>  * one-tuples will get forced down to a single line, even if they
>> started on multiple lines
>> >
>> > I don't know what this means. One-tuples are very rare anyway, so it
>> probably doesn't matter much.
>>
>> If you write this:
>>
>> FOO = (
>>     'bar',
>> )
>>
>> black will make it be
>>
>> FOO = ('bar',)
>>
>> even though there's a trailing comma, because it's the one place where a
>> trailing comma is required in Python. Does that make sense?
>>
>
> The example makes sense, but why would we want a one-tuple on multiple
> lines?

I think `directsymbols` in import-checker.py is an example that I noticed  
in D6342.  Maybe it can be turned into a list.
Matt Harbison - May 9, 2019, 3 a.m.
On Wed, 08 May 2019 15:42:08 -0400, Augie Fackler <raf@durin42.com> wrote:

>
>
>> On Dec 6, 2018, at 23:21, Matt Harbison <mharbison72@gmail.com> wrote:
>>
>> On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <yuya@tcha.org>  
>> wrote:
>>
>>> On Tue, 4 Dec 2018 10:06:24 -0500, Augie Fackler wrote:
>>>>
>>>> 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)
>>>
>>> My vote: 3 > 4 > 2 > 1
>>>
>>> I'm not super enthusiastic about 100%-machine-forced formatting. I like
>>> consistency level provided by e.g. astyle command. clang-format is  
>>> pretty
>>> good IMHO, but the black seems to sacrifice the code readability.
>>
>> +1.
>>
>> That said, I got used to longnamesthataresmooshedtogether, so I can  
>> probably adjust to anything after awhile.
>
> I've gotten frustrated with yapf and explored autopep8 - yapf is  
> frustrating, and autopep8 isn't very good at wrapping lines.  
> Fortunately, at PyCon I was able to convince ambv that  
> trailing-comma-implies-multiline was a good feature, and the code for  
> that is under review.
>
> https://phab.mercurial-scm.org/D6342 is an updated version of the  
> blackening preview with my black patch applied. I'm very happy with the  
> results (compare with the previous round - in particular imports are  
> cleaner.) It's not 100% what we want, specifically:
>
>  * imports of a single name will still get forced down to a single line
>  * one-tuples will get forced down to a single line, even if they  
> started on multiple lines
>
> Overall I'm much happier with the results now - I can make another  
> change that blackens more files if people are curious and not opposed to  
> moving forward with black. Once we can reach a consensus on a formatter,  
> we can think about running byteify-strings on everything. Realistically  
> byteifying strings will require us to be on an auto-formatter because of  
> all the too-long lines it will otherwise create.

I think this looks a lot better too.  Thanks for your work on this.
via Mercurial-devel - May 9, 2019, 3:02 a.m.
*From: *Matt Harbison <mharbison72@gmail.com>
*Date: *Wed, May 8, 2019, 19:56
*To: *Augie Fackler, Martin von Zweigbergk
*Cc: *Mercurial-devel, Gregory Szorc

On Wed, 08 May 2019 17:13:30 -0400, Martin von Zweigbergk
> <martinvonz@google.com> wrote:
>
> > *From: *Augie Fackler <raf@durin42.com>
> > *Date: *Wed, May 8, 2019 at 2:05 PM
> > *To: *martinvonz
> > *Cc: *Matt Harbison, Mercurial-devel, Gregory Szorc
> >
> >
> >>
> >> > On May 8, 2019, at 16:53, martinvonz <martinvonz@google.com> wrote:
> >> >
> >> >>  * one-tuples will get forced down to a single line, even if they
> >> started on multiple lines
> >> >
> >> > I don't know what this means. One-tuples are very rare anyway, so it
> >> probably doesn't matter much.
> >>
> >> If you write this:
> >>
> >> FOO = (
> >>     'bar',
> >> )
> >>
> >> black will make it be
> >>
> >> FOO = ('bar',)
> >>
> >> even though there's a trailing comma, because it's the one place where a
> >> trailing comma is required in Python. Does that make sense?
> >>
> >
> > The example makes sense, but why would we want a one-tuple on multiple
> > lines?
>
> I think `directsymbols` in import-checker.py is an example that I noticed
> in D6342.  Maybe it can be turned into a list.
>

I had not seen that example, but that's the type of uses I had imagined.
And I had imagined that they could just use a list, as you said.

>
Matt Harbison - May 9, 2019, 3:06 a.m.
On Wed, 08 May 2019 22:28:10 -0400, Gregory Szorc  
<gregory.szorc@gmail.com> wrote:

> On Wed, May 8, 2019 at 12:42 PM Augie Fackler <raf@durin42.com> wrote:
>
>>
>>
>> > On Dec 6, 2018, at 23:21, Matt Harbison <mharbison72@gmail.com> wrote:
>> >
>> > On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <yuya@tcha.org>
>> wrote:
>> >
>> >> On Tue, 4 Dec 2018 10:06:24 -0500, Augie Fackler wrote:
>> >>>
>> >>> 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)
>> >>
>> >> My vote: 3 > 4 > 2 > 1
>> >>
>> >> I'm not super enthusiastic about 100%-machine-forced formatting. I  
>> like
>> >> consistency level provided by e.g. astyle command. clang-format is
>> pretty
>> >> good IMHO, but the black seems to sacrifice the code readability.
>> >
>> > +1.
>> >
>> > That said, I got used to longnamesthataresmooshedtogether, so I can
>> probably adjust to anything after awhile.
>>
>> I've gotten frustrated with yapf and explored autopep8 - yapf is
>> frustrating, and autopep8 isn't very good at wrapping lines.  
>> Fortunately,
>> at PyCon I was able to convince ambv that  
>> trailing-comma-implies-multiline
>> was a good feature, and the code for that is under review.
>>
>> https://phab.mercurial-scm.org/D6342 is an updated version of the
>> blackening preview with my black patch applied. I'm very happy with the
>> results (compare with the previous round - in particular imports are
>> cleaner.) It's not 100% what we want, specifically:
>>
>>  * imports of a single name will still get forced down to a single line
>>  * one-tuples will get forced down to a single line, even if they  
>> started
>> on multiple lines
>>
>> Overall I'm much happier with the results now - I can make another  
>> change
>> that blackens more files if people are curious and not opposed to moving
>> forward with black. Once we can reach a consensus on a formatter, we can
>> think about running byteify-strings on everything. Realistically  
>> byteifying
>> strings will require us to be on an auto-formatter because of all the
>> too-long lines it will otherwise create.
>>
>
> I'm pretty happy with the state of D6342.
>
> Out of curiosity, is Black opinionated about single versus double quotes?
> AFAICT it seems to preserve whatever is there? That somewhat surprises  
> me.

https://phab.mercurial-scm.org/D5064 seems to show that it did care.  I've  
gotten into the habit of single quotes, so I'm glad this round didn't  
change them around.
via Mercurial-devel - May 9, 2019, 3:08 a.m.
*From: *Gregory Szorc <gregory.szorc@gmail.com>
*Date: *Wed, May 8, 2019, 19:32
*To: *Augie Fackler
*Cc: *mercurial-devel@mercurial-scm.org, Matt Harbison

On Wed, May 8, 2019 at 12:42 PM Augie Fackler <raf@durin42.com> wrote:
>
>>
>>
>> > On Dec 6, 2018, at 23:21, Matt Harbison <mharbison72@gmail.com> wrote:
>> >
>> > On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <yuya@tcha.org>
>> wrote:
>> >
>> >> On Tue, 4 Dec 2018 10:06:24 -0500, Augie Fackler wrote:
>> >>>
>> >>> 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)
>> >>
>> >> My vote: 3 > 4 > 2 > 1
>> >>
>> >> I'm not super enthusiastic about 100%-machine-forced formatting. I like
>> >> consistency level provided by e.g. astyle command. clang-format is
>> pretty
>> >> good IMHO, but the black seems to sacrifice the code readability.
>> >
>> > +1.
>> >
>> > That said, I got used to longnamesthataresmooshedtogether, so I can
>> probably adjust to anything after awhile.
>>
>> I've gotten frustrated with yapf and explored autopep8 - yapf is
>> frustrating, and autopep8 isn't very good at wrapping lines. Fortunately,
>> at PyCon I was able to convince ambv that trailing-comma-implies-multiline
>> was a good feature, and the code for that is under review.
>>
>> https://phab.mercurial-scm.org/D6342 is an updated version of the
>> blackening preview with my black patch applied. I'm very happy with the
>> results (compare with the previous round - in particular imports are
>> cleaner.) It's not 100% what we want, specifically:
>>
>>  * imports of a single name will still get forced down to a single line
>>  * one-tuples will get forced down to a single line, even if they started
>> on multiple lines
>>
>> Overall I'm much happier with the results now - I can make another change
>> that blackens more files if people are curious and not opposed to moving
>> forward with black. Once we can reach a consensus on a formatter, we can
>> think about running byteify-strings on everything. Realistically byteifying
>> strings will require us to be on an auto-formatter because of all the
>> too-long lines it will otherwise create.
>>
>
> I'm pretty happy with the state of D6342.
>

So am I. I don't like the changes it makes to scmposix.py, but I assume
there is no way that black will change that (since it would change too many
existing users' code). I can easily live with it.


> Out of curiosity, is Black opinionated about single versus double quotes?
> AFAICT it seems to preserve whatever is there? That somewhat surprises me.
>

That surprises me too. I'm pretty sure an earlier version of that patch
changed them all to double quotes. Augie, is that something you changed in
black?


> Also, I've mentioned this in other threads but I think it needs to be
> repeated here: I view making the source code Python 3 native (read: no
> source transforming module importer) as a prerequisite to removing the
> "beta" label from Python 3 support. This is because the source transforming
> is making some packaging problems very difficult. Because we need to use
> bytes literals, I think it is easier for us to adopt a source formatter and
> reformat the repo than to do everything by hand. So, the sooner we can get
> a style formatter in place, the sooner we can make meaningful progress
> towards making the source code Python 3 native and making Mercurial stable
> on Python 3.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Augie Fackler - May 9, 2019, 3:08 a.m.
> On May 8, 2019, at 23:06, Matt Harbison <mharbison72@gmail.com> wrote:
> 
> On Wed, 08 May 2019 22:28:10 -0400, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> 
>> On Wed, May 8, 2019 at 12:42 PM Augie Fackler <raf@durin42.com> wrote:
>> 
>> 
>> I'm pretty happy with the state of D6342.
>> 
>> Out of curiosity, is Black opinionated about single versus double quotes?
>> AFAICT it seems to preserve whatever is there? That somewhat surprises me.
> 
> https://phab.mercurial-scm.org/D5064 seems to show that it did care.  I've gotten into the habit of single quotes, so I'm glad this round didn't change them around.

It does care - I passed the -S flag that skips string normalization so the change would have less noise in it. My ideal world is we do the blackening as two passes:

 * Just whitespace/etc changes

 * Convert all strings to doublequotes

once those are both done, in theory reformatting a file is as easy as just running `black` on it since we'd have the pyproject.toml in place.
Augie Fackler - May 9, 2019, 3:09 a.m.
> On May 8, 2019, at 23:08, Martin von Zweigbergk <martinvonz@google.com> wrote:
> 
>> Out of curiosity, is Black opinionated about single versus double quotes? AFAICT it seems to preserve whatever is there? That somewhat surprises me.
> 
> That surprises me too. I'm pretty sure an earlier version of that patch changed them all to double quotes. Augie, is that something you changed in black?

It's a CLI flag - you can skip the normalization, but eventually in the name of consistency I'd like to formatterize that too.
Gregory Szorc - May 9, 2019, 3:26 p.m.
> On May 8, 2019, at 20:09, Augie Fackler <raf@durin42.com> wrote:
> 
> 
> 
>>> On May 8, 2019, at 23:08, Martin von Zweigbergk <martinvonz@google.com> wrote:
>>> 
>>> Out of curiosity, is Black opinionated about single versus double quotes? AFAICT it seems to preserve whatever is there? That somewhat surprises me.
>> 
>> That surprises me too. I'm pretty sure an earlier version of that patch changed them all to double quotes. Augie, is that something you changed in black?
> 
> It's a CLI flag - you can skip the normalization, but eventually in the name of consistency I'd like to formatterize that too.

Got it. And I’m fine doing it that way.

Is there any way to normalize on single quotes? I’m lazy and don’t like pressing the shift key :)

But I can live with double quotes. Especially if running black on file save will correct my laziness for me.
Augie Fackler - May 9, 2019, 3:51 p.m.
> On May 9, 2019, at 11:26, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> 
> 
> 
>> On May 8, 2019, at 20:09, Augie Fackler <raf@durin42.com> wrote:
>> 
>> 
>> 
>>>> On May 8, 2019, at 23:08, Martin von Zweigbergk <martinvonz@google.com> wrote:
>>>> 
>>>> Out of curiosity, is Black opinionated about single versus double quotes? AFAICT it seems to preserve whatever is there? That somewhat surprises me.
>>> 
>>> That surprises me too. I'm pretty sure an earlier version of that patch changed them all to double quotes. Augie, is that something you changed in black?
>> 
>> It's a CLI flag - you can skip the normalization, but eventually in the name of consistency I'd like to formatterize that too.
> 
> Got it. And I’m fine doing it that way.
> 
> Is there any way to normalize on single quotes? I’m lazy and don’t like pressing the shift key :)

Nope. It's black. You can have any color quotes you want, as long as they're doublequotes.

> But I can live with double quotes. Especially if running black on file save will correct my laziness for me.

If we standardize on black, I'll also put a file in contrib that will be the relevant `hg fix` configuration. It's glorious.

Patch

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.
_______________________________________________
Mercurial-devel mailing list