Seen from the other side

Posted Feb 9, 2025 1:37 UTC (Sun) by mathstuf (subscriber, #69389)
In reply to: Seen from the other side by wtarreau
Parent article: The selfish contributor revisited

> Everyone prefers being explained by a human why their work was sub-perfect and was fixed rather than having a bot reject it for obscure reasons.

In my case, the robot can also *do* the formatting fixes (I also don't like the "CI says formatting is wrong" that doesn't offer a fix or at least a diff I can apply), so there's no real "rejection" here nevermind for "obscure" reasons.

My experience is that it allows me more time as a reviewer to focus on the actual technical content of the patch rather than nitpicking over minutae (which is attention-consuming and a great way to make very-chatty PR threads…GitLab at least allows collapsing things usefully though; I don't know how Github-based reviews deal with its behaviors there).

Even as a contributor, I like using CI to improve my patches before roping a human into the loop.

Seen from the other side

Posted Feb 9, 2025 15:16 UTC (Sun) by Wol (subscriber, #4433) [Link] (3 responses)

> In my case, the robot can also *do* the formatting fixes (I also don't like the "CI says formatting is wrong" that doesn't offer a fix or at least a diff I can apply), so there's no real "rejection" here nevermind for "obscure" reasons.

Those people who don't like the project's default format (and I would definitely enforce one) should simply define their own formatter to run on checkout. Then everybody wins - code in the repository is consistently formatted to the repository's rules, and contributors work with code formatted to their rules.

Cheers,
Wol

Seen from the other side

Posted Feb 9, 2025 16:15 UTC (Sun) by mathstuf (subscriber, #69389) [Link] (2 responses)

I do wish that editors had a "make it look nice" view, but that requires reliable formatting tools (which don't exist for every language). It's not something I think should be project-specific.

In any case, I have kind of inured myself to many objectionable formatting quirks in projects I drive-by contribute to (e.g., GNU-style indentation or inter-line alignment in tab-based projects) and other times I'm just not familiar enough with the language to know how some idiom should be formatted (e.g., comment placement/wrapping or Ruby's closures). No, the issue is that I have better things to focus on when coding than whitespace tweezing or line wrapping rules; these are things we *can* reliably outsource to automation. I hope that reviewers do too.

Seen from the other side

Posted Feb 9, 2025 22:19 UTC (Sun) by wtarreau (subscriber, #51152) [Link] (1 responses)

The problem with automated tools is that it molests patches, and you end up with 1000 lines of changes instead of 3, which completely ruins your review, and that you definitely cannot commit without manually fixing to look like the original file. I agree that spaces/tabs are most of a semantic stuff than anything else that could theoretically be handled fine on the client, except that no two clients are equally configurable and you end up having to accept variations. That's where automatic reformatting of code produced in another editor makes things worse.

Seen from the other side

Posted Feb 10, 2025 7:38 UTC (Mon) by mathstuf (subscriber, #69389) [Link]

Yes, if you do not do a sweeping change to make the formatter happy with the entire codebase, you'll end up reformatting whole files when it gets touched the first time. We have the entire repository formatted en masse to make sure this doesn't happen. These commits are authored by our automation user and can be ignored for blaming purposes via `blame.ignoreRevsFile`[1]. We used to have the (IMO, awful) Whitesmith brace style and at some point decided to use something that editors can actually help with (Allman) to avoid having to think about it anymore.

[1] https://git-scm.com/docs/git-config#Documentation/git-config.txt-blameignoreRevsFile