|
|
Subscribe / Log in / New account

Seen from the other side

Seen from the other side

Posted Feb 7, 2025 6:03 UTC (Fri) by wtarreau (subscriber, #51152)
Parent article: The selfish contributor revisited

I really agree with many of James' observations and points. Sometimes you're yourself on the side of the one having to accept changes that you don't like much, and you know by being used to that, that your comments might give you a label of PA or manipulator. This often happens when you disagree with something for various reasons but try to to hurt the submitter, so you keep your dissatisfaction for you and take the patch, but the problem is that it creates a precedent, with something imperfect that serves as an example of how to properly contribute. It can be small issues like coding style etc for example, or an approach that you're not certain how it will evolve over the long term.

What I found in this case is to address yourself the small unimportant stuff (e.g. fix typos or indent), mentioning it when taking the change and presenting it as something normal, so that the person is not surprised to see their contribution modified, and if there's nothing to fix but you're not much happy with the change, but have no technical argument against it (often we're driven by feeling itself driven by experience), just openly mention it: "I sense something might go wrong by proceeding like this, but I can't tell what; since I have no technical argument, let's merge it, and if later we figure it really causes a problem, we can revert it". It generally makes the contributor more careful in the future about the goals of the project, and if the suspected problem finally happens and something needs to be changed, the person doesn't perceive it as something bad since it was already envisioned. Usually that makes that contributor feel trusted and come back with more contributions. And even the problems you envisioned are often related to a slight change of direction that was clashing with your approach, but the contributor can continue that change and finally replace an older approach with something much better in the end.

This is a good example of long-term win/win driven by trust. It's difficult though because as a project leader you're also the one responsible for the project not to steer in directions that don't match the community's expectations, but by making public statements like this, you bring the community with you, or the community will contest your decision and you can roll it back if there are good reasons.


to post comments

Seen from the other side

Posted Feb 8, 2025 0:16 UTC (Sat) by mathstuf (subscriber, #69389) [Link] (7 responses)

> What I found in this case is to address yourself the small unimportant stuff (e.g. fix typos or indent), mentioning it when taking the change and presenting it as something normal, so that the person is not surprised to see their contribution modified

Why not have this done in an automated way? It relieves the reviewer from having to consider it and the contributor can work with CI before looping a human into the loop.

Seen from the other side

Posted Feb 8, 2025 1:36 UTC (Sat) by neilbrown (subscriber, #359) [Link] (6 responses)

> Why not have this done in an automated way?

Because small-talk is important. It can help build connection and trust.
"I took your patch seriously and here are some comments" says something quite different to "nobody bothered to look at your patch because you misspelled 'colour'."

Seen from the other side

Posted Feb 8, 2025 12:03 UTC (Sat) by wtarreau (subscriber, #51152) [Link] (5 responses)

> Because small-talk is important. It can help build connection and trust.
Exactly! It's super important IMHO to build trust with the community.

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.

Also when you see the same shocking patterns happen often in submissions, you can start to think about documenting them or even relax your coding style if that's not dramatic.

Seen from the other side

Posted Feb 9, 2025 1:37 UTC (Sun) by mathstuf (subscriber, #69389) [Link] (4 responses)

> 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


Copyright © 2025, Eklektix, Inc.
Comments and public postings are copyrighted by their creators.
Linux is a registered trademark of Linus Torvalds