From dev-return-49876-archive-asf-public=cust-asf.ponee.io@couchdb.apache.org Thu Jun 3 14:19:09 2021 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mxout1-he-de.apache.org (mxout1-he-de.apache.org [95.216.194.37]) by mx-eu-01.ponee.io (Postfix) with ESMTPS id 0434E180643 for ; Thu, 3 Jun 2021 16:19:09 +0200 (CEST) Received: from mail.apache.org (mailroute1-lw-us.apache.org [207.244.88.153]) by mxout1-he-de.apache.org (ASF Mail Server at mxout1-he-de.apache.org) with SMTP id A590F61BF7 for ; Thu, 3 Jun 2021 14:19:06 +0000 (UTC) Received: (qmail 1999 invoked by uid 500); 3 Jun 2021 14:19:05 -0000 Mailing-List: contact dev-help@couchdb.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@couchdb.apache.org Delivered-To: mailing list dev@couchdb.apache.org Received: (qmail 1987 invoked by uid 99); 3 Jun 2021 14:19:05 -0000 Received: from mailrelay1-he-de.apache.org (HELO mailrelay1-he-de.apache.org) (116.203.21.61) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 03 Jun 2021 14:19:05 +0000 Received: from mail-io1-f42.google.com (mail-io1-f42.google.com [209.85.166.42]) by mailrelay1-he-de.apache.org (ASF Mail Server at mailrelay1-he-de.apache.org) with ESMTPSA id 982B63E848 for ; Thu, 3 Jun 2021 14:19:04 +0000 (UTC) Received: by mail-io1-f42.google.com with SMTP id q7so6486852iob.4 for ; Thu, 03 Jun 2021 07:19:04 -0700 (PDT) X-Gm-Message-State: AOAM5308NiLkfiTqqN576d752kEx+Y4UmIKQxH95lxcbZwqqcTmAIIrN YHxJQvefUZ/NtAiO03GGc+DSwkRhDmHxvOZRSh0= X-Google-Smtp-Source: ABdhPJwyuqSXonACTpOWBYi2GAmqSBhWxKi9SUAyggF/P63jAnpxMhpDolRPGQyC4os21LUlwjxq9bCT3iZOWELJB2E= X-Received: by 2002:a6b:3c06:: with SMTP id k6mr29436403iob.113.1622729943361; Thu, 03 Jun 2021 07:19:03 -0700 (PDT) MIME-Version: 1.0 References: <1958d593-f0b2-f2c0-3ce3-8e3356159263@apache.org> In-Reply-To: From: =?UTF-8?B?QmVzc2VueWVpIEJhbMOhenMgRG9uw6F0?= Date: Thu, 3 Jun 2021 16:18:51 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: Reformat src files with `erlfmt` on `main` To: dev@couchdb.apache.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Thank you for relaying Paul's approval, @Robert Newson ! > asks if we've confirmed that the abstract syntax tree is unaffected (i.e,= the changes are purely cosmetic and make no difference to the compiled art= ifacts). The script format_all [1] gets md5sum for the binaries of the files to be changed and does the same after formatting and reports whether the hashes match or not. If the script is written correctly, it's proven that the changes are purely cosmetic and make no difference to the artifacts. Donat [1]: https://github.com/apache/couchdb/pull/3568/files#diff-f2ea79847140720= 18b8a19daabd2dbc8df9f662af94849ffa7eb60b54bc711f0 On Thu, Jun 3, 2021 at 3:20 PM Robert Newson wrote: > > As Paul's involuntary amanuensis he says "go for it" but asks if we've co= nfirmed that the abstract syntax tree is unaffected (i.e, the changes are p= urely cosmetic and make no difference to the compiled artifacts). > > B. > > > On 2 Jun 2021, at 09:18, Bessenyei Bal=C3=A1zs Don=C3=A1t wrote: > > > >> My only nose-wrinkle was at `->` being placed on its own line under so= me > > circumstances > > I counted too many occurrences of that to add ignores for them (and peo= ple > > would probably forget adding them on new code which would result in a m= ixed > > state). > > If there are no objections, I'll go ahead with merging it with the > > controversial `->`s on newlines (as advantages seem to outweigh this > > drawback). As I mentioned earlier, if we can get a config option or cha= nge > > to erlfmt, we can always do a quick reformat. > > > >> local git hook > > I couldn't find a nice way to do it, so I can open a ticket to do that > > later. The PR adds it to CI and people can run the checks (and the > > formatter) themselves locally. > > > > I have not received a +>=3D0 from Paul, but as it's been more than a we= ek now > > I'll merge the PR assuming consent. (The PR is already approved on GitH= ub.) > > The change is not irreversible and I'd be happy to either revert or adj= ust > > if necessary. > > > > Thank you all for the support and the contribution! > > > > > > Donat > > > > > > On Fri, May 28, 2021 at 4:31 PM Ilya Khlopotov wrot= e: > > > >>> Can it also be set up as a local git hook etc? > >> > >> Few complications here: > >> 1) CouchDB codebase is not 100% resides in a single repository > >> 2) Which hook manager to use given differences in platforms we support= and > >> the fact that none of the hook managers support multiple repositories. > >> There are multiple options: > >> > >> - https://github.com/frankywahl/super_hooks > >> - https://github.com/Arkweid/lefthook > >> - https://github.com/pre-commit/pre-commit > >> > >> Do we need a separate ML discussion which hook manager to use? > >> > >> Another option is to update configure or rebar.config.script to place > >> files (or links) in `.git/hooks/pre-commit`. > >> > >> Best regards, > >> iilyak > >> > >> On 2021/05/21 12:25:53, Robert Newson wrote: > >>> Hi, > >>> > >>> My only nose-wrinkle was at `->` being placed on its own line under s= ome > >> circumstances. The rest looked good. I agree that uniformity of format= ting > >> is a very good thing and this reformat is long overdue. > >>> > >>> Agree with Donat that the formatting should be enforced by CI tools s= o > >> there=E2=80=99s no backsliding. Can it also be set up as a local git h= ook etc? > >>> > >>> B. > >>> > >>>> On 21 May 2021, at 12:46, Bessenyei Bal=C3=A1zs Don=C3=A1t > >> wrote: > >>>> > >>>> Hi All, > >>>> > >>>> I believe I've only seen +>=3D0s so far so I intend to (in the follo= wing > >> order): > >>>> * wait for an ok from @Robert Newson and @Paul J. Davis > >>>> * add `erlfmt-ignore`s if necessary to #3568 > >>>> * add a check to CI (ideally via `make`) to ensure `erlfmt` is +1 on > >>>> the PRs in #3568 > >>>> * create a PR for 3.x analogous to #3568 > >>>> > >>>> Please let me know if I missed anything. > >>>> > >>>> > >>>> Donat > >>>> > >>>> > >>>> On Fri, May 21, 2021 at 2:27 AM Joan Touzet wrot= e: > >>>>> > >>>>> In general I am +0.5 on the entire thing, but would like to see Bob > >>>>> Newson or Paul Davis speak up. In the past they've been the most vo= cal > >>>>> about code formatting standards, and I'd at least like to see a +0 > >> from > >>>>> both of them. > >>>>> > >>>>> -Joan > >>>>> > >>>>> On 20/05/2021 11:53, Ilya Khlopotov wrote: > >>>>>> Good idea Donat!!! > >>>>>> > >>>>>> Even though I disagree with some of the choices made by erlfmt I > >> appreciate consistency it provides. > >>>>>> The choices are logical. I really love that every decision is > >> documented and properly discussed. I did read PR in its entirety and i= n > >> fact was not even noticed the ugly `->` in the beginning of the line c= loser > >> to the end of the review process. > >>>>>> I do believe our wetware would adjust in no time to new formatting= . > >> Given how easy it is to reason about. I agree with Donat's observation= that > >> we are spending too much time and emphasis on formatting issues every = time > >> we review PRs. I do believe it is a machine job to provide consistent > >> formatting. We humans are better at other things. All in all I vote fo= r > >> adopting `erlfmt` for both 3.x and main. > >>>>>> > >>>>>> Also thank you Donat for providing validation scripts to make sure > >> the re-formatted code compiles to the same beam files. > >>>>>> > >>>>>> Best regards, > >>>>>> iilyak > >>>>>> > >>>>>> > >>>>>> On 2021/05/18 18:13:14, Bessenyei Bal=C3=A1zs Don=C3=A1t > >> wrote: > >>>>>>> Hi dev@couchdb, > >>>>>>> > >>>>>>> To eliminate the need for formatting-related comments and thus > >>>>>>> unnecessary cycles in PRs, I've invested a little time to see if = we > >>>>>>> could use a formatter on `main` [1]. > >>>>>>> The PR reformats `.erl` files in `src` and the script [2] include= d > >>>>>>> shows that the compiled binaries match "before" and "after". > >>>>>>> The formatter used in the PR is `erlfmt` [3] which is an opiniona= ted > >>>>>>> [4] tool so it's more of a "take it or leave it" as-is. (We could > >> try > >>>>>>> using other formatters if we want in case people want formatting = but > >>>>>>> think the choices `erlfmt` makes are unacceptable.) > >>>>>>> Some members of the CouchDB dev community already left some great > >>>>>>> comments on the PR and I haven't seen any strong opposition so fa= r, > >>>>>>> but I wanted to make sure more people are aware of this. > >>>>>>> If you have any questions, comments or concerns (or objections), > >>>>>>> please let me know. > >>>>>>> > >>>>>>> > >>>>>>> Thank you, > >>>>>>> > >>>>>>> Donat > >>>>>>> > >>>>>>> > >>>>>>> [1]: https://github.com/apache/couchdb/pull/3568 > >>>>>>> [2]: > >> https://github.com/apache/couchdb/pull/3568/files#diff-7adfbc2d8dba4d4= ff49ff2b760b81c006097f20f412ea2007f899042fd0de98a > >>>>>>> [3]: https://github.com/WhatsApp/erlfmt > >>>>>>> [4]: > >> https://github.com/WhatsApp/erlfmt#comparison-with-other-erlang-format= ters > >>>>>>> > >>> > >>> > >> >