5 ms·

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, f

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.

WmyEE0UsWAwC2i 5y ago
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.
- svnpenn 5y ago
  > 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.
  - deleted 5y ago
    [deleted]
  - xyzzy_plugh 5y ago
    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.
    - bastardoperator 5y ago
      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.
    - usr1106 5y ago
      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>
      - ptx 5y ago
        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-authors/ https://github.blog/2018-01-29-commit-together-with-co-autho...
otterley 5y ago
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.
- LorenzA 5y ago
  I find this an interesting concept. How do you do this in practice add a number to every comment and explain your system somewhere?
  - otterley 5y ago
    Yes, that’s right. Just a (1/10) or (8/10) at the end of my note.
- bryanrasmussen 5y ago
  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?)
  - otterley 5y ago
    In practice, the histogram looks like a camel back :)
klyrs 5y ago
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
tda 5y ago
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. See also https://jeremyfelt.com/2016/03/12/pieter-hintjens-building-open-source-communities/ https://jeremyfelt.com/2016/03/12/pieter-hintjens-building-o...
edem 5y ago
I just wrote a comment where I outline almost the same tactics. I think this is the most sensible strategy.

WmyEE0UsWAwC2i 5y ago

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.

svnpenn 5y ago
> 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.
- deleted 5y ago
  [deleted]
- xyzzy_plugh 5y ago
  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.
  - bastardoperator 5y ago
    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.
  - usr1106 5y ago
    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>
    - ptx 5y ago
      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-authors/ https://github.blog/2018-01-29-commit-together-with-co-autho...

otterley 5y ago

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.

LorenzA 5y ago
I find this an interesting concept. How do you do this in practice add a number to every comment and explain your system somewhere?
- otterley 5y ago
  Yes, that’s right. Just a (1/10) or (8/10) at the end of my note.
bryanrasmussen 5y ago
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?)
- otterley 5y ago
  In practice, the histogram looks like a camel back :)

klyrs 5y ago

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

tda 5y ago

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. See also https://jeremyfelt.com/2016/03/12/pieter-hintjens-building-open-source-communities/ https://jeremyfelt.com/2016/03/12/pieter-hintjens-building-o...

edem 5y ago

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