Hacker News Reader

Comment by pengaru | Hacker News Reader

If the PR is technically correct, just violates coding style or smells a bit like of hacky trash, I merge it anyways. As long as it's not breaking the build, functionality, or bisectability, just go with it. You can always chase it with a cleanup commit of your own.

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.

View on HN

Pieter Hintjens (author of ZeroMQ) wrote and spoke a lot about merging liberally. Great advice IMHO.

> 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.

I've found "importance scoring" to be a useful trick in improving my relationships with teammates and open-source contributors, while preserving my sanity. When I give feedback, I add a score from 1-10 to my comments, where 1 is "trivial nit, take it or leave it," and 10 is "there is no way I will merge this because it's fundamentally broken or would otherwise be a huge mistake." It helps me balance my perfectionist nature against the need for forward progress, and contributors appreciate this guidance because it helps prioritize where further efforts are needed.

I find this an interesting concept. How do you do this in practice add a number to every comment and explain your system somewhere?

Yes, that’s right. Just a (1/10) or (8/10) at the end of my note.

I just go with an important or not important for something I understand, I mean 6/10 I guess is important enough you don't want to merge it unless someone fixes it? And for something I don't understand I of course ask for clarification (by not understand I mean can you clarify why you did this bit?)

In practice, the histogram looks like a camel back :)

This seems like the more ethical approach with regards to giving credit to the original author.

I've seen projects (on GitHub, won't call them out) get bad reputation when the maintainer implements code previously submitted as a PR.

> won't call them out

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.

Conversely I think that's fine. There's no obligation to accept every PR as-is. Ultimately Bram volunteers to be responsible for supporting the project, so he can do whatever he wants.

I don't understand how this is the whole point of GitHub. I feel like GitHub has exacerbated misguided expectations that every contribution is worthy and that being rejected should feel bad.

The entire point of github is collaboration. If you're going to take others people code, you should at least attribute them. If you look at neovim where I imagine most serious vim users myself included have migrated to, you can see the projects first order of business:

Simplify maintenance and encourage contributions

One project has 700+ contributors, the other is less than 120. So no, it's not looking "fine" from a user perspective.

At least he should give credit to the original author.

Not aware there would be a standardized header for that, but you can always invent your own ones.

   Original-idea-from: a b <a.b@c.com>

There is "Co-authored-by" which is supported on GitHub [1] and seems appropriate if the maintainer is basing the solution on someone's code.

[1] https://github.blog/2018-01-29-commit-together-with-co-autho...

I think it's worth investing some time into people, to teach them about testing and documentation standards. If they can't / won't play along, my first approach is to do a PR into their branch. If that stalls out, I'll make a duplicate* PR from my fork of theirs, maintaining their credit and keeping my source tree up to standards.

Edit: * that almost never happens, it appears that github now supports changing the base of a PR

I just wrote a comment where I outline almost the same tactics. I think this is the most sensible strategy.

Reply on news.ycombinator.com