nickva commented on pull request #3568:
URL: https://github.com/apache/couchdb/pull/3568#issuecomment-842569320
Nice work @bessbd.
* I like that it is consistent and it auto-formats the code that is a big
plus and it removes a good amount of back-and-forth during reviews
* Good call to check the hashes before and after, that's a neat trick!
* Some erlfmt choices are odd, such as putting `->` on a line by itself. I
don't like it but not a deal breaker compared to other benefits
* In general I would prefer 80 columns (that is configurable `erlfmt
--print-width=80`) but if most people would want to stick with the default 100
that's ok too. It seems, based on how case nesting is reformatted (always
indented with 4 spaces) some places like auth handlers end up being squished to
the right even with 100 columns. With 80 it would be worse so I can live with
100 columns.
* I don't like the spacing between functions as much, I prefer one line
between function head clauses, and two lines between functions. However, if
erlfmt means having consistency I am ok sticking with erlfmt's way.
Based on @iilyak's scale (-5...+2), I'd go with +1 as an average of my
choices. But also think we should wait perhaps until erlfmt reaches 1.0 and
maybe gather more feedback (possibly with a post on dev@couchdb?). Also wonder
if there any chance convincing erlfmt authors to change their mind about a few
of the most disliked features here such as `->` being on its own line.
Another issue is being able to cherry-pick commits between `3.x` and `main`.
We have two recent PRs, for example, (Erlang 24 support and a pending one for
moving chttpd/httpd settings around) that would apply neatly to both branches,
but if we reformat one branch those won't apply as easily. Not a deal breaker,
but perhaps we would run erlfmt on both main and 3.x at the same time and maybe
coordinating with pending PR authors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
