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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
27 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Augie Fackler-2
In https://phab.mercurial-scm.org/D5064 we're talking about using the `black` formatter from Łukasz Langa on hg. This will imply a few style changes to our codebase, notably:

 * We'll use " everywhere instead of '
 * Import formatting will change a bit
 * We'll have two blank lines between top-level items in a module

Everything else is, as best I can tell, a minor style change of sorts. My preference at this point is to land the change and just give our formatting entirely to black, even if at the edges I disagree with some of black's decisions. Does anyone have a strong opinion _against_ using black to format our code?
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Pulkit Goyal
I am +1 on using black.

Adding to what Augie said, we plan to preserve the annotate history by commiting the changes with something like "# skip-blame" in commit message. We can then skip those revisions while annotating using the `--skip` flag of `hg annotate` which accepts a revset, something like `hg annotate --skip "desc('# skip-blame')"`.

Maybe we should use "# skip-blame black-changes" to mark those changes as black specific and to differentiate between py3 and black changes. (We already use "# skip-blame" for py3 related trivial changes.)

On Tue, Nov 27, 2018 at 6:54 PM Augie Fackler <[hidden email]> wrote:
In https://phab.mercurial-scm.org/D5064 we're talking about using the `black` formatter from Łukasz Langa on hg. This will imply a few style changes to our codebase, notably:

 * We'll use " everywhere instead of '
 * Import formatting will change a bit
 * We'll have two blank lines between top-level items in a module

Everything else is, as best I can tell, a minor style change of sorts. My preference at this point is to land the change and just give our formatting entirely to black, even if at the edges I disagree with some of black's decisions. Does anyone have a strong opinion _against_ using black to format our code?
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Augie Fackler-2


> On Nov 27, 2018, at 11:09, Pulkit Goyal <[hidden email]> wrote:
>
> I am +1 on using black.
>
> Adding to what Augie said, we plan to preserve the annotate history by commiting the changes with something like "# skip-blame" in commit message. We can then skip those revisions while annotating using the `--skip` flag of `hg annotate` which accepts a revset, something like `hg annotate --skip "desc('# skip-blame')"`.
>
> Maybe we should use "# skip-blame black-changes" to mark those changes as black specific and to differentiate between py3 and black changes. (We already use "# skip-blame" for py3 related trivial changes.)

Indeed. Note that if we do the black transition, we should probably also do the mass-addition of b prefixes on our string literals at the same time (in a separate commit, obviously) so that people only have painful rebases once and not twice.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Pulkit Goyal


On Tue, Nov 27, 2018 at 7:10 PM Augie Fackler <[hidden email]> wrote:


> On Nov 27, 2018, at 11:09, Pulkit Goyal <[hidden email]> wrote:
>
> I am +1 on using black.
>
> Adding to what Augie said, we plan to preserve the annotate history by commiting the changes with something like "# skip-blame" in commit message. We can then skip those revisions while annotating using the `--skip` flag of `hg annotate` which accepts a revset, something like `hg annotate --skip "desc('# skip-blame')"`.
>
> Maybe we should use "# skip-blame black-changes" to mark those changes as black specific and to differentiate between py3 and black changes. (We already use "# skip-blame" for py3 related trivial changes.)

Indeed. Note that if we do the black transition, we should probably also do the mass-addition of b prefixes on our string literals at the same time (in a separate commit, obviously) so that people only have painful rebases once and not twice.

If nobody opposes black formatting, are we going to do that as a single commit or a list of commits with a commit for each file? I prefer per-file as that will be easy to review. but that will lead to lots of commits. :(

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Gregory Szorc
In reply to this post by Augie Fackler-2
On Tue, Nov 27, 2018 at 8:13 AM Augie Fackler <[hidden email]> wrote:


> On Nov 27, 2018, at 11:09, Pulkit Goyal <[hidden email]> wrote:
>
> I am +1 on using black.
>
> Adding to what Augie said, we plan to preserve the annotate history by commiting the changes with something like "# skip-blame" in commit message. We can then skip those revisions while annotating using the `--skip` flag of `hg annotate` which accepts a revset, something like `hg annotate --skip "desc('# skip-blame')"`.
>
> Maybe we should use "# skip-blame black-changes" to mark those changes as black specific and to differentiate between py3 and black changes. (We already use "# skip-blame" for py3 related trivial changes.)

Indeed. Note that if we do the black transition, we should probably also do the mass-addition of b prefixes on our string literals at the same time (in a separate commit, obviously) so that people only have painful rebases once and not twice.

IMO the annotation should be "# auto-reformat" or something along those lines: you should be annotating the action that was taken, not the side-effects you purport you want to take from it. When skipping revisions for annotate, we can use a revset with all the relevant annotations. e.g. `desc("# auto-reformat") | desc("# py3-compat")`. But we've already started using "# skip-blame" so perhaps this ship has sailed.

Speaking of ships that have sailed, I would prefer we use "annotate" instead of "blame," as the former doesn't have the negative connotations of the latter.

I agree with Augie that a mass-addition of b prefixes should be done at the same time in a separate commit from reformatting.

I have no preferences whether we should have a single massive commit or split things up by file.

I also have some issues with black's formatting. I dislike black's use of double quotes for strings (maybe a holdover from my knowledge of shell, Perl, and PHP, where different quotes matter). And I wish it didn't collect imports on the same line. But my objections are insignificant compared to the benefits of having consistently formatted source code. The research in this area shows that the actual style matters less than consistency. If you really care about a style, you can configure your editor to apply your personal source format on file load and the official style format on save. You don't get to do that on remote code review, hgweb, or when looking at diffs from `hg` output. But it's a mitigation if you care that much.

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Yuya Nishihara
On Tue, 27 Nov 2018 15:42:21 -0800, Gregory Szorc wrote:
> I also have some issues with black's formatting. I dislike black's use of
> double quotes for strings (maybe a holdover from my knowledge of shell,
> Perl, and PHP, where different quotes matter). And I wish it didn't collect
> imports on the same line.

I don't care ' vs ", but I agree that the import style isn't nice. Merge
conflicts will be more likely to occur because of that.

> But my objections are insignificant compared to
> the benefits of having consistently formatted source code. The research in
> this area shows that the actual style matters less than consistency.

Can we somehow reject patches automatically if they disagree with the
formatter? I'm not interested in fixing test-check-* failure in flight.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Augie Fackler-2
In reply to this post by Gregory Szorc
On Tue, Nov 27, 2018 at 6:42 PM Gregory Szorc <[hidden email]> wrote:

>
> On Tue, Nov 27, 2018 at 8:13 AM Augie Fackler <[hidden email]> wrote:
>>
>>
>>
>> > On Nov 27, 2018, at 11:09, Pulkit Goyal <[hidden email]> wrote:
>> >
>> > I am +1 on using black.
>> >
>> > Adding to what Augie said, we plan to preserve the annotate history by commiting the changes with something like "# skip-blame" in commit message. We can then skip those revisions while annotating using the `--skip` flag of `hg annotate` which accepts a revset, something like `hg annotate --skip "desc('# skip-blame')"`.
>> >
>> > Maybe we should use "# skip-blame black-changes" to mark those changes as black specific and to differentiate between py3 and black changes. (We already use "# skip-blame" for py3 related trivial changes.)

>>
>> Indeed. Note that if we do the black transition, we should probably also do the mass-addition of b prefixes on our string literals at the same time (in a separate commit, obviously) so that people only have painful rebases once and not twice.
>
>
> IMO the annotation should be "# auto-reformat" or something along those lines: you should be annotating the action that was taken, not the side-effects you purport you want to take from it. When skipping revisions for annotate, we can use a revset with all the relevant annotations. e.g. `desc("# auto-reformat") | desc("# py3-compat")`. But we've already started using "# skip-blame" so perhaps this ship has sailed.

auto-reformat seems fine, I'm happy to have both.

>
> Speaking of ships that have sailed, I would prefer we use "annotate" instead of "blame," as the former doesn't have the negative connotations of the latter.
>
> I agree with Augie that a mass-addition of b prefixes should be done at the same time in a separate commit from reformatting.
>
> I have no preferences whether we should have a single massive commit or split things up by file.
>
> I also have some issues with black's formatting. I dislike black's use of double quotes for strings (maybe a holdover from my knowledge of shell, Perl, and PHP, where different quotes matter).

> And I wish it didn't collect imports on the same line.

I think we could work around this by having one import per import statement, eg

from mercurial import hg
from mercurial import ui as uimod

I don't remember why we don't do that today. Is it performance or aesthetics?

> But my objections are insignificant compared to the benefits of having consistently formatted source code. The research in this area shows that the actual style matters less than consistency. If you really care about a style, you can configure your editor to apply your personal source format on file load and the official style format on save. You don't get to do that on remote code review, hgweb, or when looking at diffs from `hg` output. But it's a mitigation if you care that much.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Augie Fackler-2
In reply to this post by Yuya Nishihara
On Wed, Nov 28, 2018 at 7:13 AM Yuya Nishihara <[hidden email]> wrote:

>
> On Tue, 27 Nov 2018 15:42:21 -0800, Gregory Szorc wrote:
> > I also have some issues with black's formatting. I dislike black's use of
> > double quotes for strings (maybe a holdover from my knowledge of shell,
> > Perl, and PHP, where different quotes matter). And I wish it didn't collect
> > imports on the same line.
>
> I don't care ' vs ", but I agree that the import style isn't nice. Merge
> conflicts will be more likely to occur because of that.
>
> > But my objections are insignificant compared to
> > the benefits of having consistently formatted source code. The research in
> > this area shows that the actual style matters less than consistency.
>
> Can we somehow reject patches automatically if they disagree with the
> formatter? I'm not interested in fixing test-check-* failure in flight.

Two possibilities here:

1) We configure `hg fix` and then maintainers can just have a habit of
running `hg fix` whenever they see test-check* failures. Not great,
but relatively low overhead.

2) We write a phabricator (and probably also email, though I'd expect
email contributors to be savvier) bot that checks for some basics like
code formatting consistency and robo-comments on the change.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Gregory Szorc
In reply to this post by Augie Fackler-2
On Thu, Nov 29, 2018 at 1:28 PM Augie Fackler <[hidden email]> wrote:
On Tue, Nov 27, 2018 at 6:42 PM Gregory Szorc <[hidden email]> wrote:
>
> On Tue, Nov 27, 2018 at 8:13 AM Augie Fackler <[hidden email]> wrote:
>>
>>
>>
>> > On Nov 27, 2018, at 11:09, Pulkit Goyal <[hidden email]> wrote:
>> >
>> > I am +1 on using black.
>> >
>> > Adding to what Augie said, we plan to preserve the annotate history by commiting the changes with something like "# skip-blame" in commit message. We can then skip those revisions while annotating using the `--skip` flag of `hg annotate` which accepts a revset, something like `hg annotate --skip "desc('# skip-blame')"`.
>> >
>> > Maybe we should use "# skip-blame black-changes" to mark those changes as black specific and to differentiate between py3 and black changes. (We already use "# skip-blame" for py3 related trivial changes.)

>>
>> Indeed. Note that if we do the black transition, we should probably also do the mass-addition of b prefixes on our string literals at the same time (in a separate commit, obviously) so that people only have painful rebases once and not twice.
>
>
> IMO the annotation should be "# auto-reformat" or something along those lines: you should be annotating the action that was taken, not the side-effects you purport you want to take from it. When skipping revisions for annotate, we can use a revset with all the relevant annotations. e.g. `desc("# auto-reformat") | desc("# py3-compat")`. But we've already started using "# skip-blame" so perhaps this ship has sailed.

auto-reformat seems fine, I'm happy to have both.

>
> Speaking of ships that have sailed, I would prefer we use "annotate" instead of "blame," as the former doesn't have the negative connotations of the latter.
>
> I agree with Augie that a mass-addition of b prefixes should be done at the same time in a separate commit from reformatting.
>
> I have no preferences whether we should have a single massive commit or split things up by file.
>
> I also have some issues with black's formatting. I dislike black's use of double quotes for strings (maybe a holdover from my knowledge of shell, Perl, and PHP, where different quotes matter).

> And I wish it didn't collect imports on the same line.

I think we could work around this by having one import per import statement, eg

from mercurial import hg
from mercurial import ui as uimod

I don't remember why we don't do that today. Is it performance or aesthetics?

Each "import X" or "from X import Y" statement is essentially converted into a call to __import__(). So having a single statement with multiple "from" values means fewer calls to __import__(). This in turn means fewer lookups of the parent package and other actions that are performed in the bowels of the importer. (Keep in mind we have a custom importer for delay loading and for source transformation, so there may be a substantial amount of code that is skipped by coalescing the imports). What that performance translates to, I'm not sure.

Perhaps someone could run black on the entire source tree and compare the execution time of a simple command to see if it matters in reality?

We could, of course, rewrite the source code as part of importing (like we do on Python 3) to the "faster" form if there are performance implications.
 

> But my objections are insignificant compared to the benefits of having consistently formatted source code. The research in this area shows that the actual style matters less than consistency. If you really care about a style, you can configure your editor to apply your personal source format on file load and the official style format on save. You don't get to do that on remote code review, hgweb, or when looking at diffs from `hg` output. But it's a mitigation if you care that much.

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Anton Shestakov-3
In reply to this post by Augie Fackler-2
On Tue, 27 Nov 2018 10:52:56 -0500
Augie Fackler <[hidden email]> wrote:

> In https://phab.mercurial-scm.org/D5064 we're talking about using the `black` formatter from Łukasz Langa on hg. This will imply a few style changes to our codebase, notably:
>
>  * We'll use " everywhere instead of '
>  * Import formatting will change a bit
>  * We'll have two blank lines between top-level items in a module
>
> Everything else is, as best I can tell, a minor style change of sorts. My preference at this point is to land the change and just give our formatting entirely to black, even if at the edges I disagree with some of black's decisions. Does anyone have a strong opinion _against_ using black to format our code?

It's still not clear to me why we're using black instead of a linter if
it just does minor style changes and ' -> ". I also like consistency,
but do we care about only ever using " for strings? Because D5064
indeed only highlights that we already have consistent code as far as
people are concerned, it's just one tool that's upset about tiniest
things -- that's how I see it.

Or how about YAPF, why not? At least it's configurable. So far I've
only heard that "we wouldn't argue about style rules if somebody else
chose them for us". We have clang-format rules, and I remember they
were worked out on the mailing list, and people who work on this
project decided on what's best for it. But now we don't like that idea?
What happened?
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Anton Shestakov-3
In reply to this post by Augie Fackler-2
On Thu, 29 Nov 2018 16:29:30 -0500
Augie Fackler <[hidden email]> wrote:

> On Wed, Nov 28, 2018 at 7:13 AM Yuya Nishihara <[hidden email]> wrote:
> > Can we somehow reject patches automatically if they disagree with the
> > formatter? I'm not interested in fixing test-check-* failure in flight.
>
> Two possibilities here:
>
> 1) We configure `hg fix` and then maintainers can just have a habit of
> running `hg fix` whenever they see test-check* failures. Not great,
> but relatively low overhead.
>
> 2) We write a phabricator (and probably also email, though I'd expect
> email contributors to be savvier) bot that checks for some basics like
> code formatting consistency and robo-comments on the change.

If right now somebody accepts a patch that's not formatted ideally --
no big deal, we can fix it later. But if everything is black and we
take a patch that's not, then working on those files could get messy,
because black works on whole files.

We'd also be redefining what "formatted ideally" means from "looks
kinda PEP8-ish" to "just run black all the time or you'll miss a
whitespace or use a wrong quote mark". This imposes quite a lot more on
new contributions.

Considering all that, I think 1) would be a bare minimum.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Boris Feld
In reply to this post by Augie Fackler-2
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.

When doing this change, we should leverage all the available tooling
around Mercurial + source formatting. This means using `hg fix` to help
with the formatting of in-progress patches, but also the `format-source`
extensions to remove merge conflict introduced by formatting. `fix` and
`format-source` are complementary.

Using tools like format source in the Mercurial project is important for
multiple reasons:

First, it is useful, formatting introduces pervasive changes that will
conflict with all in progress contribution. And such conflict will
happen every-time we adjust the formatting. `format-source` removes
these conflict. Making contributors life easier.

Second, we have been displaying the format-source extensions in a couple
of conferences and it received a very warm welcome. Multiple people
pointing out this is the kind of tooling giving Mercurial a significant
edge over other VCS. Using it for Mercurial will help to polish it and
make sure source-code formatting becomes a first class citizen in Mercurial.


Overall, `hg fix` and `hg format-source` handle two facets of the same
issue and should be merged (or at least share a large part of code and
configuration). I know that Martijn Pieters and Danny Hooper made some
plan in that direction at the last sprint.

On 27/11/2018 16:52, Augie Fackler wrote:

> In https://phab.mercurial-scm.org/D5064 we're talking about using the `black` formatter from Łukasz Langa on hg. This will imply a few style changes to our codebase, notably:
>
>  * We'll use " everywhere instead of '
>  * Import formatting will change a bit
>  * We'll have two blank lines between top-level items in a module
>
> Everything else is, as best I can tell, a minor style change of sorts. My preference at this point is to land the change and just give our formatting entirely to black, even if at the edges I disagree with some of black's decisions. Does anyone have a strong opinion _against_ using black to format our code?
> _______________________________________________
> Mercurial-devel mailing list
> [hidden email]
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Matt Harbison-2
On Fri, 30 Nov 2018 07:25:04 -0500, Boris FELD <[hidden email]>  
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.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Augie Fackler-2
(+cc people that I saw on this thread with opinions)

> On Nov 30, 2018, at 20:35, Matt Harbison <[hidden email]> wrote:
>
> On Fri, 30 Nov 2018 07:25:04 -0500, Boris FELD <[hidden email]> 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
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Yuya Nishihara
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.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Matt Harbison-2
On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <[hidden email]> 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.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Auto-formatting with yapf (was: Re: Auto-formatting code with black - object now if you have a strong opinion)

Augie Fackler-2


> On Dec 6, 2018, at 23:21, Matt Harbison <[hidden email]> wrote:
>
> On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <[hidden email]> 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?
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: Auto-formatting with yapf (was: Re: Auto-formatting code with black - object now if you have a strong opinion)

Matt Harbison-2
On Wed, 09 Jan 2019 15:30:19 -0500, Augie Fackler <[hidden email]> wrote:

>
>
>> On Dec 6, 2018, at 23:21, Matt Harbison <[hidden email]> wrote:
>>
>> On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <[hidden email]>  
>> 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?
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: Auto-formatting with yapf (was: Re: Auto-formatting code with black - object now if you have a strong opinion)

Augie Fackler-2


> On Jan 10, 2019, at 00:14, Matt Harbison <[hidden email]> wrote:
>
> On Wed, 09 Jan 2019 15:30:19 -0500, Augie Fackler <[hidden email]> wrote:
>
>>
>>
>>> On Dec 6, 2018, at 23:21, Matt Harbison <[hidden email]> wrote:
>>>
>>> On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <[hidden email]> 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?

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: Auto-formatting with yapf (was: Re: Auto-formatting code with black - object now if you have a strong opinion)

Matt Harbison-2
On Thu, 10 Jan 2019 13:51:12 -0500, Augie Fackler <[hidden email]> wrote:

>
>
>> On Jan 10, 2019, at 00:14, Matt Harbison <[hidden email]> wrote:
>>
>> On Wed, 09 Jan 2019 15:30:19 -0500, Augie Fackler <[hidden email]>  
>> wrote:
>>
>>>
>>>
>>>> On Dec 6, 2018, at 23:21, Matt Harbison <[hidden email]> wrote:
>>>>
>>>> On Wed, 05 Dec 2018 08:23:17 -0500, Yuya Nishihara <[hidden email]>  
>>>> 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.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
12