20 ms·

Why I close pull requests

292 points by geerlingguy 10y ago

makecheck 10y ago
Also, don’t send requests out of the blue. The original maintainer has to know that you’re working on something. One reason is that your changes might collide spectacularly with other planned changes you weren’t aware of. Another reason is that the maintainer might say “no” to the entire idea, much less the implementation, and save you time. The mere creation of a fork isn’t a sufficient signal, either; the project maintainer isn’t going to treat that as a sign that you’re actually working on something. (There seem to be an insane number of forks out there that are created and never changed again, apparently used to pad résumés by having important-sounding projects listed on user profiles.)
- Skuzzzy 10y ago
  Padding resumes is a bit cynical there are many good reasons to make a fork for oneself. Like ensuring you have code you depend on in the event the project is deleted
  - mikeash 10y ago
    I've also seen people who don't understand how his stuff works, and fork projects before cloning them because they think that's the process. A relatively small number of clueless people could result in a lot of pointless forks.
    - tedunangst 10y ago
      The big button does say "fork me on github" after all.
      - mikeash 10y ago
        Indeed, if you're not quite sure what to do, then that would be an obvious one to try. And it'll work in the end, so you won't necessarily change afterwards.
    - ageofwant 10y ago
      How are you supposed to do it otherwise ? Where else are you going to push your commits to ? Its silly to clone, fiddle with git config, make a new git repo on github/lab and then push to your new 'manual fork'. Just use the fork button.
      - mikeash 10y ago
        99% of the time, when I clone an open source repository, I just want to use it, not make new commits to it. Obviously, if you want your own copy to commit to, then use the fork button. That's what it's for. But lots of people use the fork button even when their use case is read-only.
        ageofwant 10y ago
        I sometimes use the fork button just to get a copy I may or may not play with over the weekend.
        mikeash 10y ago
        How does that help? Are you just using the presence of the fork in your account as a sort of bookmark?
        ageofwant 10y ago
        Yes, a complete and fully featured bookmark that won't disappear. It costs me absolutely nothing, and is ready to go when and if I need it. Why wouldn't I do that if I could ?
        mikeash 10y ago
        It makes perfect sense. This isn't what I've seen some people do, though.
        fapjacks 10y ago
        Yeah, this is what I do, too. Also keep in mind, though, that you'll need to exfil your fork if you think it might disappear at all from Github. They yank the original repo and all of its forks for example if they get a DMCA notice. Years ago, I set up automation that pulls repos that I've forked, so for me, the act of forking ensures I get a copy of the code imported into my personal Gitlab instance on my home LAN.
- joejev 10y ago
  Why not send requests out of the blue? This happens to me all the time and I don't mind. It means someone cares enough to try to fix their problems instead of dumping it on me.
  - marcosdumay 10y ago
    The problem is on the other side. If you don't want the push, the sender just wasted a some work.
    - rpedela 10y ago
      Not necessarily. They obviously need that change so it wasn't wasted. However they have to maintain the change themselves if it isn't merged.
      - asdkjfsad888 10y ago
        How do you know they obviously need the change by making a decision away from the project management discussions? Perhaps a solution has been discussed and is incoming from a regular developer? They could deprecate the module where the change is "obviously" needed in a larger fix. Frame it a different way: You're basing the needs of a project on your view of it, which, minus discussing things with project mgmt beforehand, may be incomplete.
        Nadya 10y ago
        You completely misunderstood who was needing the fix. In this case, it was the person who spent time fixing it. They likely fixed it because they need the fix. Doesn't matter if the entire module becomes deprecated. They'll stay back with their fix (likely). Doesn't matter if another solution is inbound, they needed the fix now and not when a patch lands. And since they already took the time to fix it - little time is lost sending a PR, regardless if it gets merged or not. If it is merged? Sweet you helped the project out (most likely). If not? Well a small bummer that you'll need to maintain your own patch(es).
- meesterdude 10y ago
  what on earth are you talking about? I pull a project into my codebase, find it has bugs or missing features so i fork it, make my changes and PR. If I fork, write the code and follow the outlined contribution guidelines (which often do not include "ask for permission") then I'm doing what I should in the OSS world.
- mgbmtl 10y ago
  Presumably you mean: "don't send requests [for new features] out of the blue"? I would add: if you do, don't expect the maintainer to merge it. Once it's merged, it becomes their technical debt. Nonetheless, as a maintainer I always appreciate a pull-request, and as a developer I might fork/apply non-merged patches for projects where I needed that feature as well. In some cases, since they saved me a lot of time, I might fix their abandoned/closed patch and re-submit it upstream.
- alaaibrahim 10y ago
  Why Not? I see a problem that I can fix, I do that, and then send it back to the original maintainer, making sure that I'm not breaking tests if they exist, and providing a good description of the change. Then it's up to the maintainer to either merge it, or throw it away. Of course, I don't have any problem with getting my PRs closed, I'm not the one who would end up maintaining the code.
  - asdkjfsad888 10y ago
    > I see a problem that I can fix... Remember, they said "out of the blue" which means "without prior contact to discuss doing so." This exact reason was explained in the parent post: maybe the maintainers and project owners don't see it as a problem or are already working on something. It's not just about being a machine that can crank out fixes to the individual issues in software wherever you see them. It's about participating in a social project intended to fix a larger scale problem. > Of course, I don't have any problem with getting my PRs closed, I'm not the one who would end up maintaining the code. A reason alone not to send PR's "out of the blue". If you're just looking to pump and dump, I'd rather not involve you in the process, only to have to do work to fix/rm code down the road. But I mean, above all, it's your time to waste.
    - akerl_ 10y ago
      The post calls out major changes as deserving a chat first. My primary workflow that results in PRs is: 1) I find an interesting project, 2) I find something it doesn't do the way I want (a bug, or a config tweak), 3) I patch my fork to fix that, 4) I open up a PR incase the maintainer does think it's worth merging into upstream. What's the upside of me opening an issue first to chat about it? The maintainer still has to burn the time to think about it, but without seeing the code that I'm proposing, which is already written because I already needed it to scratch my itch. If they think it's worth of merging, woo, we merge it. If they want the problem fixed a different way, cool, one of us writes the PR that makes that happen. If they don't want the change, I keep using my fork.
      - bcuzitsppl8383 10y ago
- manarth 10y ago
  Given the scenario that I am using open-source software X, and that I have made changes to the software to suit my requirements, and that I believe those changes might be useful to others, I can either: 1. Contribute those changes up-stream, by sending a pull request. 2. Publicise the changes, by keeping my fork, and/or talking about it in a blog post. 3. Keep quiet, and say nothing. 4. Send an email to the original developer, suggesting I may have some useful changes, and asking whether I should send a PR. If I start working on changes without notifying the original maintainer, well, I might do work that's useless, that maintains no value, that isn't sustainable. But that's my loss. If I send a PR, the net loss is the maintainer's time to review my PR (if they choose to). Many open-source contributions stem from sending PRs out of the blue. You're right to say that it can be an inefficient mechanism in some circumstances; it's just that a lot of developers are OK with that.
  - StavrosK 10y ago
    If you've already made the changes, then by all means, just send a PR. The worst thing that can happen is that it won't get merged. If you're considering doing some work, please talk to the maintainer first. Otherwise it might lead to unhappiness all around, because, as a maintainer, I don't like turning down all this work you've done for free any more than you do, but there's not much choice when it's a net negative (for any of the reasons in the article).
    - geerlingguy 10y ago
      Exactly. Too often I get a large PR that I call a 'code dump'. at least give a couple paragraphs of explanation behind the changes. Sometimes a conversation can start in a PR, but it's more rare that results in merged code than if there was an issue first.
      - Klathmon 10y ago
        Yeah, I semi-recently had to fork a library, make some big changes to get it working for us, then made an issue with the main repo basically saying "Hey I needed this and don't have time to do it properly, Here is what I did, and I ham-fistedly ripped out everything i'm not using. Look at these few files for an example of the core change that's needed. I'm willing to help work on a real solution later, but I can't right now" The maintainer saw what I did, and was able to easily make the change so it conformed to their style, their architecture, and they worked with me later so I could make the doc changes. I felt like that worked magnitudes better than when someone makes a big PR with a potentially controversial change and lets the maintainer decide what to do with it.
- Manishearth 10y ago
  I think a better way to put this is "don't send requests out of the blue if you care about them getting merged". I often hack on software that I use to make it do something I need. I then try to upstream the code, if I think that it's something others may want too. I don't particularly care about it being merged -- my attitude to this is "Here's something I found useful, if other people think it's good please take it". I'm willing to put some effort into cleaning up the patch to make it submission-worthy, but if it doesn't get merged, it doesn't get merged. I'll keep it up in a fork, and that's about it. I don't care about it getting upstreamed enough that I will open a dialogue before I start to work on it. It's a feature I want, and I will be working on it regardless of how the dialogue goes down. The only way a dialogue can help me is by giving me implementation advice, but quite often I've already figured out a way to do it which is sufficient for my purposes (hacky or otherwise). If there's a feature that you actually care about existing in upstream, then you should totally open an issue first and discuss it.
- UK-AL 10y ago
  I work on many things for software projects for my own personal gain. I don't particularly care if someone is planning to do something similar in the future. I need this thing now, for whatever project I'm currently doing. I implement it, and sometimes I may send a PR to see if the maintainer wants it. I don't particularly care if it gets accepted or not. However I think giving them the option to pull if they want it, is a good idea.
- acjohnson55 10y ago
  At the same time, sometimes, the best argument for a big new idea is _code_ showing that it works in reality. Obviously, if you're going to do a major refactor on your own, you're risking that work being thrown away. So, it's certainly a risk.
caconym_ 10y ago
Just stopping by to point out that there is a typo in the first sentence of the article, I think: in "I maintain over many", the "over" should not be there. Now I will read the article. :)
- ycmbntrthrwaway 10y ago
  Also "the bus factor is high". Should be "low", as it is 1 for most projects.
  - NickBusey 10y ago
    I'm not sure if you're just being pedantic, but "the bus factor is high" in this instance means there is a high level of risk to the health of the OSS projects he maintains if he were to get hit by a bus.
    - lgas 10y ago
      I don't think he's being pedantic, it's the opposite of the general usage. The bus factor is the number of people that have to be hit by a bus to cause real problems for your project. Higher is better.
      - CaptSpify 10y ago
        Indeed! > The "bus factor" is the minimum number of team members that have to suddenly disappear from a project before the project stalls due to lack of knowledgeable or competent personnel. https://en.wikipedia.org/wiki/Bus_factor https://en.wikipedia.org/wiki/Bus_factor
- hk__2 10y ago
  You should report this to the blog author; not to HN readers.
  - caconym_ 10y ago
    My assumption is that the OP, "geerlingguy", and the post's actual author Jeff Geerling, are one and the same. Not unreasonable, I think. If he wants to post his work on HN and not read the discussion (which is obviously going to be relevant to his work), that's on him.
sytse 10y ago
Wow, managing over 160 projects? I can imagine that he has to close quickly. At GitLab we have a written down definition of done so people know what should be in their merge request, see https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#definition-of-done https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBU... And our merge request coaches try to get people over the finish line instead of closing. But that are full time people on a single project. Maintaining 160 projects is a whole different ballgame.
- geerlingguy 10y ago
  I'll _usually_ leave it open if it looks interesting to me, passes tests, and I know I'll get back to it in my next round of reviews (every quarter or half year, I spend an evening or two of my dedicated OSS time reviewing each project's open issues and PRs to clean things up). I try to at least indicate if it's close to ready for merge or if it will need a bit more work, and don't often immediately close a PR.
  - sytse 10y ago
    At GitLab I think our policy is to already leave feedback too, even when we close it immediately. Of course we have more resources than one volunteer running 160 projects.
yegle 10y ago
At Google, if you want to implement new features (or large refactoring), you'll need to write a design doc. In which, you should answer questions your reviewers might ask (common questions like: why do you want to do this, what are the alternatives, how components interactive with each other before/after your change). This is something like Python's PEP: you need a proposal to convince your reviewer that you have put thought into your change. Real world examples of these design docs can be found at https://github.com/golang/proposal https://github.com/golang/proposal
- fdsfsaa 10y ago
  Requiring permission to do work is the enemy of progress and engineering dignity. It creates a presumption of incompetence and an atmosphere of low trust that punishes people who want to push the envelope of what's possible. Google's design document culture is bad. Google has succeeded in spite of it. In my experience, having worked at many large tech companies, design documents obfuscate, not enlighten. They become increasingly out-of-date as the code evolves, creating anti-documentation that makes it take longer to understand code. Yes, yes, people should update design documents as the code evolves. Everyone knows that in practice, nobody updates old design documents. Design documents make it too easy for other developers to shoot down ideas. Sometimes the worth of code isn't apparent until it's made. It's far too easy for someone to comment "this will never work" on a proposed change. It's much harder for someone to deny benchmarks attached to a proposed change. It's too easy for reviewers to knock out functionality. Design documents turn every feature into a half-assed, lowest-common-denominator risk-minimized barely-adequate shell of itself. The real reason everyone at Google writes design documents is that promotion committees demand documents as "evidence of complexity". No design document, no impact. No impact, no promotion. Code is just code. Bad changes can be backed out. It's much better to move fast and iterate quickly than to create an illusion of care and add friction to every aspect of the development process. Up-front design of software just does not work. If it did, waterfall project planning would be successful. These questions you highlight --- Why are you making this change? What impact will it have? --- can be asked during review of actual code. There is no need to build a speedbump, not if you trust your people. Developers should be able to choose to create design documents and solicit feedback. Most changes don't need this process. You should trust developers to know what changes require a more extensive discussion and which ones don't. A culture that requires permissions and signoffs before work can begin is a culture that leaves products stagnant for years.
dclowd9901 10y ago
You know what would be cool? If I could create a fork off the project i was using. Then I write a feature I need in that project. it becomes a PR, but then the fork is also automatically (if possible) updated whenever the main branch is updated. If it can't be updated automatically you are notified to update your fork against the upstream changes. This would have many benefits, including easy testing of PRs, forks that don't stale, and overall helping closing the loop on open PRs.
- mikepurvis 10y ago
  I don't know about "automatically", since all the hashes would change, but I definitely do wish that Github had better tools for managing long term forks, especially web workflows for: - rebase this branch on upstream - resolve merge/rebase conflicts - make my master be upstream's master with PRs X, Y, and Z applied to it in that order. I find the last one in particular to be awful, where I'm iterating on multiple PRs with an upstream, and keeping them separate for the sake of review, but wanting to test as a group to validate the end-to-end functionality. It can be a huge hassle cherry picking and rebasing commits between branches.
  - daurnimator 10y ago
    > resolve merge/rebase conflicts This was added to github rather recently
  - akkartik 10y ago
    Have you considered just constantly rebasing as you pull? This works well for private forks, but it may also be doable for public forks if you explain what's going on in the Readme. Just an idea. I know it goes against everything we learn about Git, but perhaps it's not so bad in this particular situation.
    - mikepurvis 10y ago
      Yeah, I mean that's basically what you do, but it becomes a huge pain if you're making more than one contribution upstream. You feature-branch-1, feature-branch-2, and then the upstream master with those two branches merged into it. When you update either one of the two PRs, you need to rewind the upstream master and reapply the two merges onto it. Maybe that doesn't sound so bad, but it feels super awkward to me.
fourthark 10y ago
I know it's not best practice, but I leave them open. For years. They may be fixable, they may be useful to someone. I've no need to reject them unless I really think they're a bad idea. I'm sure this can be frustrating to users and contributors, but I also see it as a way of encouraging forks. "I haven't had a chance to review this, but you might try PR #NN..." The most useful ones get replaced by better versions by other people, and eventually merged.
- flurdy 10y ago
  That depends on if you respond with a comment that indicates it's not likely to be merged. Then it is understandable, otherwise it is just rude and confusing.
  - fourthark 10y ago
    Well, I usually say that it needs tests, and in my crowd that's equivalent. :-/
- unixhero 10y ago
  Yeah. But it is not the way PR and Git is intended
  - palunon 10y ago
    Git is intended to work over email. How do you close an email PR ?
    - pm215 10y ago
      You reply to the email saying 'sorry, but we can't accept your code because...'. (The shortest way to do this is to just reply saying 'NAK', but I agree with Rusty Russell that nak-mails are generally unfriendly and a bad idea: http://ozlabs.org/~rusty/index.cgi/tech/2007-05-04.html http://ozlabs.org/~rusty/index.cgi/tech/2007-05-04.html)
- sfifs 10y ago
Sir_Cmpwn 10y ago
I also maintain a great number of projects (perhaps more) and generally I give feedback on why a PR is unacceptable and leave it open until it's resolved. Sometimes I'll close it a year or two later.
- geerlingguy 10y ago
  Definitely a valid approach. Usually people digging for historic non-merged requests will still find the closed requests either in Google or GitHub search, so I close them by default just to keep open PRs more focused.
deleted 10y ago
[deleted]
saw-lau 10y ago
Upvoted for introducing me to the term 'bus factor.' https://en.wikipedia.org/wiki/Bus_factor https://en.wikipedia.org/wiki/Bus_factor
realstuff 10y ago
Sounds like an arrogant bastard.
- dang 10y ago
  You've posted quite a few uncivil and/or unsubstantive comments to HN. That's not what this site is for, so please stop doing that. You can read the following to get an idea of what we're looking for here: https://news.ycombinator.com/newsguidelines.html https://news.ycombinator.com/newsguidelines.html https://news.ycombinator.com/newswelcome.html https://news.ycombinator.com/newswelcome.html
Marazan 10y ago
The fact they have to state they won't accept pull requests that break the build astounds me. Who is submitting pull requests that break the build? That is akin to trolling.
- vickychijwani 10y ago
  There are junior/newbie devs unfamiliar with the concept of CI etc, who will often submit working code without updated tests, and in that case it's up to the community/maintainer to point them in the right direction and help get their PR merged. An important first step to lay the foundation for future contributions :)
emmab 10y ago
If a given PR would be acceptable except for its maintenance burden, and you do not expect the PR submitter to provide sufficient help with maintenance to compensate, you could request the difference from them as payment for acceptance of the PR.
radarsat1 10y ago
One thing that I find different about github vs previously (e.g on sourceforge) when you had to sort of sign up to be part of the group to propose a change, is that people feel a lot more free to suggest out of the blue quite impactful but ultimately rather superficial changes to a project. They argue and argue to get these changes accepted, and then disappear. On several projects I've been on, I get issues or pull requests proposing to change the entire build system of a project. As you know, for C/C++ projects, the build system can be non-trivial, and maybe many years have gone into getting it to work well. And as things change, we adapt it. But as soon as it's not the flavour of the week, you get github requests suggesting to change to a completely different one, to suit some or other system's needs. A tweak here, a tweak there, or an entire overhaul being proposed from people who haven't contributed to the actual code base at all. These infrastructure "suggestions" from people who aren't invested in the project but love to play with scripts built up "around" the code get very annoying. I don't know what the difference is exactly but it didn't happen with such frequency when things were more oriented around mailing lists. I've now got 3 projects that have at least two build systems each, because of random people's preferences. That is a lot of extra work to maintain that is orthogonal to the actual project source code. I've started closing PRs that make infrastructural changes that I don't want to be responsible for, unless I can get the submitter to promise he'll be around for a while to maintain it. I've also started forcing people to put such changes in subfolders so that it's clear which one is the "supported" system. And I haven't shied from "assigning" subsequent bugs back to the original PR submitter. But sometimes that doesn't even solicit a response. People: if you are going to suggest switching a project to a completely different build system, and then disappear and not promise to maintain said system, please think twice about changing something just because it doesn't suit your preferences of the week.
digi_owl 10y ago
Between the responses here and on the more recent Chrome for business posting, i find myself wonder if there is a ever widening split between the "push to prod" web dev mentality, and the "clasic software" mentality.
- scaryclam 10y ago
  I think there's always been a "just ship it" and "let's make sure it's well tested" split. I find that there's also the cowboy coders, who tend to create nasty problems constantly pushing to production and breaking things for users, and the folks who push to production often but make sure what they've pushed is well covered by unit and functional tests, as well as having been properly designed and signed off before starting work at all. The former seem to have commented a lot on this article, which I find quite sad if it's a growing trend :(
  - digi_owl 10y ago
    I feel it is a growing trend as the net makes it easier and easier to go "release now, patch later". Except that rather than patch later it gets left in place. And then rewritten from scratch in whatever is the buzzword language of the day some years down the road as the guard changes.
j1vms 10y ago
The article brings up a lot of great points. Though people do need to be mindful lest they be writing a follow-up, "Why my project got forked, and I now sidelined."
dmuhs 10y ago
This article gives a nice view for someone who hasn't had that much experience with OSS development like myself. While I'm kinda familiar with CI systems and the concept of coverage, could someone explain to me what the author means by "happy path" in coverage? Is that considered the most used path in standard behaviour?
- geerlingguy 10y ago
  > Is that considered the most used path in standard behaviour? Yes, basically. If you're building an application that allows people to submit a contact form, then the happy path would be something like: 1. Load form 2. Verify the correct fields are there 3. Fill out form with valid data 4. Submit form 5. Verify submission went through correctly The happy path is a minimum viable test to make sure things are working. For completeness, you'd also want to make sure that input is sanitized, invalid input (e.g. non-functional email addresses) causes a form submission to fail, and layout looks correct (e.g. CSS styles applied).
deleted 10y ago
[deleted]
deleted 10y ago
[deleted]

292 points by geerlingguy 10y ago

Also, don’t send requests out of the blue. The original maintainer has to know that you’re working on something. One reason is that your changes might collide spectacularly with other planned changes you weren’t aware of. Another reason is that the maintainer might say “no” to the entire idea, much less the implementation, and save you time. The mere creation of a fork isn’t a sufficient signal, either; the project maintainer isn’t going to treat that as a sign that you’re actually working on something. (There seem to be an insane number of forks out there that are created and never changed again, apparently used to pad résumés by having important-sounding projects listed on user profiles.)

Skuzzzy 10y ago
Padding resumes is a bit cynical there are many good reasons to make a fork for oneself. Like ensuring you have code you depend on in the event the project is deleted
- mikeash 10y ago
  I've also seen people who don't understand how his stuff works, and fork projects before cloning them because they think that's the process. A relatively small number of clueless people could result in a lot of pointless forks.
  - tedunangst 10y ago
    The big button does say "fork me on github" after all.
    - mikeash 10y ago
      Indeed, if you're not quite sure what to do, then that would be an obvious one to try. And it'll work in the end, so you won't necessarily change afterwards.
  - ageofwant 10y ago
    How are you supposed to do it otherwise ? Where else are you going to push your commits to ? Its silly to clone, fiddle with git config, make a new git repo on github/lab and then push to your new 'manual fork'. Just use the fork button.
    - mikeash 10y ago
      99% of the time, when I clone an open source repository, I just want to use it, not make new commits to it. Obviously, if you want your own copy to commit to, then use the fork button. That's what it's for. But lots of people use the fork button even when their use case is read-only.
      - ageofwant 10y ago
        I sometimes use the fork button just to get a copy I may or may not play with over the weekend.
        mikeash 10y ago
        How does that help? Are you just using the presence of the fork in your account as a sort of bookmark?
        ageofwant 10y ago
        Yes, a complete and fully featured bookmark that won't disappear. It costs me absolutely nothing, and is ready to go when and if I need it. Why wouldn't I do that if I could ?
        mikeash 10y ago
        It makes perfect sense. This isn't what I've seen some people do, though.
        fapjacks 10y ago
        Yeah, this is what I do, too. Also keep in mind, though, that you'll need to exfil your fork if you think it might disappear at all from Github. They yank the original repo and all of its forks for example if they get a DMCA notice. Years ago, I set up automation that pulls repos that I've forked, so for me, the act of forking ensures I get a copy of the code imported into my personal Gitlab instance on my home LAN.
joejev 10y ago
Why not send requests out of the blue? This happens to me all the time and I don't mind. It means someone cares enough to try to fix their problems instead of dumping it on me.
- marcosdumay 10y ago
  The problem is on the other side. If you don't want the push, the sender just wasted a some work.
  - rpedela 10y ago
    Not necessarily. They obviously need that change so it wasn't wasted. However they have to maintain the change themselves if it isn't merged.
    - asdkjfsad888 10y ago
      How do you know they obviously need the change by making a decision away from the project management discussions? Perhaps a solution has been discussed and is incoming from a regular developer? They could deprecate the module where the change is "obviously" needed in a larger fix. Frame it a different way: You're basing the needs of a project on your view of it, which, minus discussing things with project mgmt beforehand, may be incomplete.
      - Nadya 10y ago
        You completely misunderstood who was needing the fix. In this case, it was the person who spent time fixing it. They likely fixed it because they need the fix. Doesn't matter if the entire module becomes deprecated. They'll stay back with their fix (likely). Doesn't matter if another solution is inbound, they needed the fix now and not when a patch lands. And since they already took the time to fix it - little time is lost sending a PR, regardless if it gets merged or not. If it is merged? Sweet you helped the project out (most likely). If not? Well a small bummer that you'll need to maintain your own patch(es).
meesterdude 10y ago
what on earth are you talking about? I pull a project into my codebase, find it has bugs or missing features so i fork it, make my changes and PR. If I fork, write the code and follow the outlined contribution guidelines (which often do not include "ask for permission") then I'm doing what I should in the OSS world.
mgbmtl 10y ago
Presumably you mean: "don't send requests [for new features] out of the blue"? I would add: if you do, don't expect the maintainer to merge it. Once it's merged, it becomes their technical debt. Nonetheless, as a maintainer I always appreciate a pull-request, and as a developer I might fork/apply non-merged patches for projects where I needed that feature as well. In some cases, since they saved me a lot of time, I might fix their abandoned/closed patch and re-submit it upstream.
alaaibrahim 10y ago
Why Not? I see a problem that I can fix, I do that, and then send it back to the original maintainer, making sure that I'm not breaking tests if they exist, and providing a good description of the change. Then it's up to the maintainer to either merge it, or throw it away. Of course, I don't have any problem with getting my PRs closed, I'm not the one who would end up maintaining the code.
- asdkjfsad888 10y ago
  > I see a problem that I can fix... Remember, they said "out of the blue" which means "without prior contact to discuss doing so." This exact reason was explained in the parent post: maybe the maintainers and project owners don't see it as a problem or are already working on something. It's not just about being a machine that can crank out fixes to the individual issues in software wherever you see them. It's about participating in a social project intended to fix a larger scale problem. > Of course, I don't have any problem with getting my PRs closed, I'm not the one who would end up maintaining the code. A reason alone not to send PR's "out of the blue". If you're just looking to pump and dump, I'd rather not involve you in the process, only to have to do work to fix/rm code down the road. But I mean, above all, it's your time to waste.
  - akerl_ 10y ago
    The post calls out major changes as deserving a chat first. My primary workflow that results in PRs is: 1) I find an interesting project, 2) I find something it doesn't do the way I want (a bug, or a config tweak), 3) I patch my fork to fix that, 4) I open up a PR incase the maintainer does think it's worth merging into upstream. What's the upside of me opening an issue first to chat about it? The maintainer still has to burn the time to think about it, but without seeing the code that I'm proposing, which is already written because I already needed it to scratch my itch. If they think it's worth of merging, woo, we merge it. If they want the problem fixed a different way, cool, one of us writes the PR that makes that happen. If they don't want the change, I keep using my fork.
    - bcuzitsppl8383 10y ago
manarth 10y ago
Given the scenario that I am using open-source software X, and that I have made changes to the software to suit my requirements, and that I believe those changes might be useful to others, I can either: 1. Contribute those changes up-stream, by sending a pull request. 2. Publicise the changes, by keeping my fork, and/or talking about it in a blog post. 3. Keep quiet, and say nothing. 4. Send an email to the original developer, suggesting I may have some useful changes, and asking whether I should send a PR. If I start working on changes without notifying the original maintainer, well, I might do work that's useless, that maintains no value, that isn't sustainable. But that's my loss. If I send a PR, the net loss is the maintainer's time to review my PR (if they choose to). Many open-source contributions stem from sending PRs out of the blue. You're right to say that it can be an inefficient mechanism in some circumstances; it's just that a lot of developers are OK with that.
- StavrosK 10y ago
  If you've already made the changes, then by all means, just send a PR. The worst thing that can happen is that it won't get merged. If you're considering doing some work, please talk to the maintainer first. Otherwise it might lead to unhappiness all around, because, as a maintainer, I don't like turning down all this work you've done for free any more than you do, but there's not much choice when it's a net negative (for any of the reasons in the article).
  - geerlingguy 10y ago
    Exactly. Too often I get a large PR that I call a 'code dump'. at least give a couple paragraphs of explanation behind the changes. Sometimes a conversation can start in a PR, but it's more rare that results in merged code than if there was an issue first.
    - Klathmon 10y ago
      Yeah, I semi-recently had to fork a library, make some big changes to get it working for us, then made an issue with the main repo basically saying "Hey I needed this and don't have time to do it properly, Here is what I did, and I ham-fistedly ripped out everything i'm not using. Look at these few files for an example of the core change that's needed. I'm willing to help work on a real solution later, but I can't right now" The maintainer saw what I did, and was able to easily make the change so it conformed to their style, their architecture, and they worked with me later so I could make the doc changes. I felt like that worked magnitudes better than when someone makes a big PR with a potentially controversial change and lets the maintainer decide what to do with it.
Manishearth 10y ago
I think a better way to put this is "don't send requests out of the blue if you care about them getting merged". I often hack on software that I use to make it do something I need. I then try to upstream the code, if I think that it's something others may want too. I don't particularly care about it being merged -- my attitude to this is "Here's something I found useful, if other people think it's good please take it". I'm willing to put some effort into cleaning up the patch to make it submission-worthy, but if it doesn't get merged, it doesn't get merged. I'll keep it up in a fork, and that's about it. I don't care about it getting upstreamed enough that I will open a dialogue before I start to work on it. It's a feature I want, and I will be working on it regardless of how the dialogue goes down. The only way a dialogue can help me is by giving me implementation advice, but quite often I've already figured out a way to do it which is sufficient for my purposes (hacky or otherwise). If there's a feature that you actually care about existing in upstream, then you should totally open an issue first and discuss it.
UK-AL 10y ago
I work on many things for software projects for my own personal gain. I don't particularly care if someone is planning to do something similar in the future. I need this thing now, for whatever project I'm currently doing. I implement it, and sometimes I may send a PR to see if the maintainer wants it. I don't particularly care if it gets accepted or not. However I think giving them the option to pull if they want it, is a good idea.
acjohnson55 10y ago
At the same time, sometimes, the best argument for a big new idea is _code_ showing that it works in reality. Obviously, if you're going to do a major refactor on your own, you're risking that work being thrown away. So, it's certainly a risk.

caconym_ 10y ago

Just stopping by to point out that there is a typo in the first sentence of the article, I think: in "I maintain over many", the "over" should not be there. Now I will read the article. :)

ycmbntrthrwaway 10y ago
Also "the bus factor is high". Should be "low", as it is 1 for most projects.
- NickBusey 10y ago
  I'm not sure if you're just being pedantic, but "the bus factor is high" in this instance means there is a high level of risk to the health of the OSS projects he maintains if he were to get hit by a bus.
  - lgas 10y ago
    I don't think he's being pedantic, it's the opposite of the general usage. The bus factor is the number of people that have to be hit by a bus to cause real problems for your project. Higher is better.
    - CaptSpify 10y ago
      Indeed! > The "bus factor" is the minimum number of team members that have to suddenly disappear from a project before the project stalls due to lack of knowledgeable or competent personnel. https://en.wikipedia.org/wiki/Bus_factor https://en.wikipedia.org/wiki/Bus_factor
hk__2 10y ago
You should report this to the blog author; not to HN readers.
- caconym_ 10y ago
  My assumption is that the OP, "geerlingguy", and the post's actual author Jeff Geerling, are one and the same. Not unreasonable, I think. If he wants to post his work on HN and not read the discussion (which is obviously going to be relevant to his work), that's on him.

sytse 10y ago

Wow, managing over 160 projects? I can imagine that he has to close quickly. At GitLab we have a written down definition of done so people know what should be in their merge request, see https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#definition-of-done https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBU... And our merge request coaches try to get people over the finish line instead of closing. But that are full time people on a single project. Maintaining 160 projects is a whole different ballgame.

geerlingguy 10y ago
I'll _usually_ leave it open if it looks interesting to me, passes tests, and I know I'll get back to it in my next round of reviews (every quarter or half year, I spend an evening or two of my dedicated OSS time reviewing each project's open issues and PRs to clean things up). I try to at least indicate if it's close to ready for merge or if it will need a bit more work, and don't often immediately close a PR.
- sytse 10y ago
  At GitLab I think our policy is to already leave feedback too, even when we close it immediately. Of course we have more resources than one volunteer running 160 projects.

yegle 10y ago

At Google, if you want to implement new features (or large refactoring), you'll need to write a design doc. In which, you should answer questions your reviewers might ask (common questions like: why do you want to do this, what are the alternatives, how components interactive with each other before/after your change). This is something like Python's PEP: you need a proposal to convince your reviewer that you have put thought into your change. Real world examples of these design docs can be found at https://github.com/golang/proposal https://github.com/golang/proposal

fdsfsaa 10y ago
Requiring permission to do work is the enemy of progress and engineering dignity. It creates a presumption of incompetence and an atmosphere of low trust that punishes people who want to push the envelope of what's possible. Google's design document culture is bad. Google has succeeded in spite of it. In my experience, having worked at many large tech companies, design documents obfuscate, not enlighten. They become increasingly out-of-date as the code evolves, creating anti-documentation that makes it take longer to understand code. Yes, yes, people should update design documents as the code evolves. Everyone knows that in practice, nobody updates old design documents. Design documents make it too easy for other developers to shoot down ideas. Sometimes the worth of code isn't apparent until it's made. It's far too easy for someone to comment "this will never work" on a proposed change. It's much harder for someone to deny benchmarks attached to a proposed change. It's too easy for reviewers to knock out functionality. Design documents turn every feature into a half-assed, lowest-common-denominator risk-minimized barely-adequate shell of itself. The real reason everyone at Google writes design documents is that promotion committees demand documents as "evidence of complexity". No design document, no impact. No impact, no promotion. Code is just code. Bad changes can be backed out. It's much better to move fast and iterate quickly than to create an illusion of care and add friction to every aspect of the development process. Up-front design of software just does not work. If it did, waterfall project planning would be successful. These questions you highlight --- Why are you making this change? What impact will it have? --- can be asked during review of actual code. There is no need to build a speedbump, not if you trust your people. Developers should be able to choose to create design documents and solicit feedback. Most changes don't need this process. You should trust developers to know what changes require a more extensive discussion and which ones don't. A culture that requires permissions and signoffs before work can begin is a culture that leaves products stagnant for years.

dclowd9901 10y ago

You know what would be cool? If I could create a fork off the project i was using. Then I write a feature I need in that project. it becomes a PR, but then the fork is also automatically (if possible) updated whenever the main branch is updated. If it can't be updated automatically you are notified to update your fork against the upstream changes. This would have many benefits, including easy testing of PRs, forks that don't stale, and overall helping closing the loop on open PRs.

mikepurvis 10y ago
I don't know about "automatically", since all the hashes would change, but I definitely do wish that Github had better tools for managing long term forks, especially web workflows for: - rebase this branch on upstream - resolve merge/rebase conflicts - make my master be upstream's master with PRs X, Y, and Z applied to it in that order. I find the last one in particular to be awful, where I'm iterating on multiple PRs with an upstream, and keeping them separate for the sake of review, but wanting to test as a group to validate the end-to-end functionality. It can be a huge hassle cherry picking and rebasing commits between branches.
- daurnimator 10y ago
  > resolve merge/rebase conflicts This was added to github rather recently
- akkartik 10y ago
  Have you considered just constantly rebasing as you pull? This works well for private forks, but it may also be doable for public forks if you explain what's going on in the Readme. Just an idea. I know it goes against everything we learn about Git, but perhaps it's not so bad in this particular situation.
  - mikepurvis 10y ago
    Yeah, I mean that's basically what you do, but it becomes a huge pain if you're making more than one contribution upstream. You feature-branch-1, feature-branch-2, and then the upstream master with those two branches merged into it. When you update either one of the two PRs, you need to rewind the upstream master and reapply the two merges onto it. Maybe that doesn't sound so bad, but it feels super awkward to me.

fourthark 10y ago

I know it's not best practice, but I leave them open. For years. They may be fixable, they may be useful to someone. I've no need to reject them unless I really think they're a bad idea. I'm sure this can be frustrating to users and contributors, but I also see it as a way of encouraging forks. "I haven't had a chance to review this, but you might try PR #NN..." The most useful ones get replaced by better versions by other people, and eventually merged.

flurdy 10y ago
That depends on if you respond with a comment that indicates it's not likely to be merged. Then it is understandable, otherwise it is just rude and confusing.
- fourthark 10y ago
  Well, I usually say that it needs tests, and in my crowd that's equivalent. :-/
unixhero 10y ago
Yeah. But it is not the way PR and Git is intended
- palunon 10y ago
  Git is intended to work over email. How do you close an email PR ?
  - pm215 10y ago
    You reply to the email saying 'sorry, but we can't accept your code because...'. (The shortest way to do this is to just reply saying 'NAK', but I agree with Rusty Russell that nak-mails are generally unfriendly and a bad idea: http://ozlabs.org/~rusty/index.cgi/tech/2007-05-04.html http://ozlabs.org/~rusty/index.cgi/tech/2007-05-04.html)
sfifs 10y ago

Sir_Cmpwn 10y ago

I also maintain a great number of projects (perhaps more) and generally I give feedback on why a PR is unacceptable and leave it open until it's resolved. Sometimes I'll close it a year or two later.

geerlingguy 10y ago
Definitely a valid approach. Usually people digging for historic non-merged requests will still find the closed requests either in Google or GitHub search, so I close them by default just to keep open PRs more focused.

deleted 10y ago

[deleted]

saw-lau 10y ago

Upvoted for introducing me to the term 'bus factor.' https://en.wikipedia.org/wiki/Bus_factor https://en.wikipedia.org/wiki/Bus_factor

realstuff 10y ago

Sounds like an arrogant bastard.

dang 10y ago
You've posted quite a few uncivil and/or unsubstantive comments to HN. That's not what this site is for, so please stop doing that. You can read the following to get an idea of what we're looking for here: https://news.ycombinator.com/newsguidelines.html https://news.ycombinator.com/newsguidelines.html https://news.ycombinator.com/newswelcome.html https://news.ycombinator.com/newswelcome.html

Marazan 10y ago

The fact they have to state they won't accept pull requests that break the build astounds me. Who is submitting pull requests that break the build? That is akin to trolling.

vickychijwani 10y ago
There are junior/newbie devs unfamiliar with the concept of CI etc, who will often submit working code without updated tests, and in that case it's up to the community/maintainer to point them in the right direction and help get their PR merged. An important first step to lay the foundation for future contributions :)

emmab 10y ago

If a given PR would be acceptable except for its maintenance burden, and you do not expect the PR submitter to provide sufficient help with maintenance to compensate, you could request the difference from them as payment for acceptance of the PR.

radarsat1 10y ago

One thing that I find different about github vs previously (e.g on sourceforge) when you had to sort of sign up to be part of the group to propose a change, is that people feel a lot more free to suggest out of the blue quite impactful but ultimately rather superficial changes to a project. They argue and argue to get these changes accepted, and then disappear. On several projects I've been on, I get issues or pull requests proposing to change the entire build system of a project. As you know, for C/C++ projects, the build system can be non-trivial, and maybe many years have gone into getting it to work well. And as things change, we adapt it. But as soon as it's not the flavour of the week, you get github requests suggesting to change to a completely different one, to suit some or other system's needs. A tweak here, a tweak there, or an entire overhaul being proposed from people who haven't contributed to the actual code base at all. These infrastructure "suggestions" from people who aren't invested in the project but love to play with scripts built up "around" the code get very annoying. I don't know what the difference is exactly but it didn't happen with such frequency when things were more oriented around mailing lists. I've now got 3 projects that have at least two build systems each, because of random people's preferences. That is a lot of extra work to maintain that is orthogonal to the actual project source code. I've started closing PRs that make infrastructural changes that I don't want to be responsible for, unless I can get the submitter to promise he'll be around for a while to maintain it. I've also started forcing people to put such changes in subfolders so that it's clear which one is the "supported" system. And I haven't shied from "assigning" subsequent bugs back to the original PR submitter. But sometimes that doesn't even solicit a response. People: if you are going to suggest switching a project to a completely different build system, and then disappear and not promise to maintain said system, please think twice about changing something just because it doesn't suit your preferences of the week.

digi_owl 10y ago

Between the responses here and on the more recent Chrome for business posting, i find myself wonder if there is a ever widening split between the "push to prod" web dev mentality, and the "clasic software" mentality.

scaryclam 10y ago
I think there's always been a "just ship it" and "let's make sure it's well tested" split. I find that there's also the cowboy coders, who tend to create nasty problems constantly pushing to production and breaking things for users, and the folks who push to production often but make sure what they've pushed is well covered by unit and functional tests, as well as having been properly designed and signed off before starting work at all. The former seem to have commented a lot on this article, which I find quite sad if it's a growing trend :(
- digi_owl 10y ago
  I feel it is a growing trend as the net makes it easier and easier to go "release now, patch later". Except that rather than patch later it gets left in place. And then rewritten from scratch in whatever is the buzzword language of the day some years down the road as the guard changes.

j1vms 10y ago

The article brings up a lot of great points. Though people do need to be mindful lest they be writing a follow-up, "Why my project got forked, and I now sidelined."

dmuhs 10y ago

This article gives a nice view for someone who hasn't had that much experience with OSS development like myself. While I'm kinda familiar with CI systems and the concept of coverage, could someone explain to me what the author means by "happy path" in coverage? Is that considered the most used path in standard behaviour?

geerlingguy 10y ago
> Is that considered the most used path in standard behaviour? Yes, basically. If you're building an application that allows people to submit a contact form, then the happy path would be something like: 1. Load form 2. Verify the correct fields are there 3. Fill out form with valid data 4. Submit form 5. Verify submission went through correctly The happy path is a minimum viable test to make sure things are working. For completeness, you'd also want to make sure that input is sanitized, invalid input (e.g. non-functional email addresses) causes a form submission to fail, and layout looks correct (e.g. CSS styles applied).

deleted 10y ago

[deleted]

deleted 10y ago

[deleted]