Just wondering how other open-source developers deal with valid by badly-written pull requests.
I usually work closely with the PR author, giving them multiple rounds of suggestions until it's up to our project's coding standards. Sometimes the PR author "drops out" of the process in frustration, as they were just trying to give us a quick fix, and didn't necessarily want to do a lot of work. In those cases, I usually thank them, and rewrite the PR (or heavily modify it). But I'm hesitant to do this in all cases, because often the PR author is excited to see their name as part of the contributors list, and/or to see their own code used in the project.
How so you deal with this kind of thing?
It's up to the PR's author to decide if they care enough to do better next time. By accepting their work and landing the change you've demonstrated an appreciation for their effort with open arms, and I feel this makes it more likely they will contribute in the future and care about doing better.
Maybe make a comment to that effect before merging as-is, creating the opportunity to clean it up themselves first.
If it's coming from a regular contributor I'm far less charitable, this approach I mostly reserve for first-time/infrequent contributors.
> merge pull requests liberally and get new contributors’ “vision of progress on record” so that they immediately become members of the community. Worry about fixing the progress later.
See also https://jeremyfelt.com/2016/03/12/pieter-hintjens-building-o...
I've seen projects (on GitHub, won't call them out) get bad reputation when the maintainer implements code previously submitted as a PR.
I will. vim/vim has been doing this for years, with Bram basically ignoring the whole point of GitHub, and putting his name on every commit. I love Vim, but that's not acceptable.
Edit: * that almost never happens, it appears that github now supports changing the base of a PR
But it would be nice if there's a way to say "look, it is what it is, and while I'm willing to make an effort, I'm not living and breathing this codebase, and I'm not planning to become a major contributor going forward, much as I like the project. Playing 20 questions is exhausting on both ends and if you'd rather just take it and completely recast it how you want, I honestly don't mind and won't take offense" without sounding like "here's some random code, IDK if it works really, good luck, lol".
I don't even care about the authorship, I'd rather have the fix than get credit for a moribund PR. Half the time I contribute pseudonymously anyway.
Deleted Comment
It's not that I will refuse to make changes, I will, within reason, it's that I'm also fine with the maintainer just taking over at any point and finishing it however they want, without any further back-and-forth. And I don't even care whose name is on it.
1. I've been on the other side of spending time trying to figure out the arcane organizational details, existing utils functions, etc only to have there be things I missed. This is deeply frustrating.
2. I'm deeply appreciative that somebody took the effort to do something on project of mine! I recognize they did that for free out of good will and don't want to make it harder for them.
3. I know the codebase much better. I can fix up/trim/format/etc properly 10x faster than the contributor can. I'd rather spend 15 mins of my own time than try to suck away an hour or two of their time. I value my time very highly but I also try to be generous and loving, and that requires some giving of my most valuable asset: my time.
Super low effort/spammy PRs do not count. I don't merge those because I don't want to incentivize more of them.
1. Fork the PR branch. 2. Make the changes I want to see upstream. 3. Create a PR into the author's branch. 4. Ask the author to review my changes. 5. When my PR is complete, finally review and merge The original PR.
This gives me the opportunity to explain my changes in commit messages, and gives the author an opportunity to push back or argue actual functionality changes I made if it's not all coding style. Now the author has license in accepting my changes. It avoids the cumbersome comment/resolve feedback loop, shows the author that their contribution is important and I'm willing to put time into making it right, and ultimately gives them credit when their PR is merged.
I guess that's messy if it's linked to issues and the connotation of "declined PR" isn't as nice.
That preserves the original author credit (and any GPG signatures), but moves things along. If that's not enabled, making a new PR based on the same commits works too.
As mentioned by sph: some projects have maintainers that have much stricter standards for outside contributors that for themselves. Speaking as a maintainer of a project that accepts contributions through PR requests, I am afraid I do indeed have such a double standard, although I try to avoid it as much as possible. I had to set some rules for myself:
- Contributors must fix outright bugs in their commits.
- Style issues are avoided by having a coding style enforced by a code formatting tool. This saves everyones time.
- If the commit is bug free (passes test suite and code review) and is a net improvement, accept it, regardless of my own plans for the project.
- If there is more that can be done to make it ideal, this can be done in a future pull request, or perhaps I will add those changes later myself.
I also try to hold myself to the same standards as I hold contributors to, and now often create a development branch myself for features so they get the same CI testing as pull requests, although I still sometimes bypass this for quick fixes.
Do you have any particular rules about performance? I worry that accepting net improvements on functionality alone could result in “death-by-a-thousand-cuts” performance issues down the line (assume it’s a project where performance is one of the deliverables).
Would you ask a contributor to perform benchmarks or some other performance validation, for example?
What you do with the performance results really depends on the kind of patch they want to commit. Is it an important bug fix? Then I would get that merged first and worry about fixing performance afterwards. And then you have those patches that improve performance in one area at the cost of decreasing it in another. In that case you might have to use your experience in what is more important to decide whether to accept or reject it.
>When code reviews are slow, several things happen: ...
>Developers start to protest the code review process. If a reviewer only responds every few days, but requests major changes to the CL each time, that can be frustrating and difficult for developers. Often, this is expressed as complaints about how “strict” the reviewer is being. If the reviewer requests the same substantial changes (changes which really do improve code health), but responds quickly every time the developer makes an update, the complaints tend to disappear. Most complaints about the code review process are actually resolved by making the process faster.
I generally err on the side of retaining the authorship when I do this kind of thing (our project follows the kernel's de-facto standard of retaining the original git author but adding a note of extra changes made in square brackets after the author's signed-off-by and before mine as the committer, which is how we record who's responsible for what parts of a change like this). If you've literally completely written a fresh change to solve the same problem a different way then that should be your authorship with some kind of reported-by or other acknowledgement to the PR author, though.