
Why I close pull requests - geerlingguy
http://www.jeffgeerling.com/blog/2016/why-i-close-prs-oss-project-maintainer-notes
======
yegle
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
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.

~~~
stinkytaco
I can't comment on Google's culture, but:

>Design documents turn every feature into a half-assed, lowest-common-
denominator risk-minimized shell of itself.

Sounds like a sentence written by someone who is an engineer and not a support
staff or a user, i.e the people who have to deal with the fallout of every
feature change and every engineering decision. I could just as easily
substitute "feature driven design" into most of your points above.

Requiring someone to put thought into the design of a product and to subject
that thought to rigorous scrutiny is, on the whole, a good thing. One that
leads to products end users want to use.

~~~
fdsfsaa
> Sounds like a sentence written by someone who is an engineer and not a
> support staff or a user

Do support staff and users sign off on design documents? "No" is the universal
answer. Are you claiming that engineers aren't reasonable human beings who can
take support staff and user concerns into account on their own? What makes you
think the people reviewing design documents can do that?

Is it that you just trust a subset of engineers to understand the big picture?
Are most engineers just drones? You shouldn't hire people who don't give
enough of a shit to take the big picture into account.

I am an engineer. I am _also_ a user. I do support my software. I've been
programming for twenty years. I find that "rigorous scrutiny" hurts more often
than it helps. Maybe this rigid scrutiny was more appropriate in a world with
release cycles measured in years, but we don't live in that world anymore.
When you ship every week, you can easily undo mistakes, and you're better off
erring toward iteration.

~~~
zardeh
>You shouldn't hire people who don't give enough of a shit to take the big
picture into account.

At a certain point you can't. I was recently asked to implement a feature for
a usecase for another engineer. It required a design doc and review by a few
representatives from related teams. He and I wrote the doc, and the initial
review was that any solution that would fix his usecase would break the
testing infrastructure for practically every other developer in the building.
He could write a few fewer loc when testing, at the cost of tests failing due
to unrelated changes.

Neither he nor I had the awareness of the impact, and realistically couldn't
have, it was neither of our responsibilities. And because I spent an hour
filling out a template document, I didn't need to waste my time doing that.

~~~
fdsfsaa
> break the testing infrastructure for practically every other developer in
> the building

Why wouldn't continuous integration have caught that bug? If a diff breaks the
build, it shouldn't land. If a diff causes tests to fail, it shouldn't land.
If a diff lands anyway and causes problems, any developer negatively affected
should be able to insta-revert the diff.

How does a design document help?

~~~
hhandoko
It helps identify potential problems / conflicts across teams even before a
single code is written (or development time committed).

~~~
fdsfsaa
I don't think so. Who looks at design documents? Your own team. If you don't
yourself catch that a change will cause problems with other teams, the
existence of a design document won't magically alert that team. If you _do_
suspect that there might be a bad interaction with another team, you can alert
that team with or without a design document.

So again: how does a design document requirement help?

~~~
zardeh
>Who looks at design documents? Your own team.

At least where I work, no. Your team, your manager, and anyone who you think
will be affected, and depending on the scale of the change, you inform
everyone that uses your tool or works on your product so that anyone can
provide comments and feedback.

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

~~~
joejev
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
The problem is on the other side. If you don't want the push, the sender just
wasted a some work.

~~~
rpedela
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
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
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).

~~~
greglindahl
This is an odd way of thinking about it. As a user of software, I like fixing
it for my needs but I'd rather make the change in a way that the maintainer
will accept, so that I don't have to maintain my patch. That means I'll tend
to open an issue first, explain that I'm willing to submit a PR, and see if
the maintainer wants to give me guidance.

~~~
rpedela
I think it depends a lot on context. If I am looking to contribute for the
sole purpose of improving the OSS project, then I follow your method. If I am
using an OSS project in a larger commercial project and I need a bugfix or
feature now, I just build it. If I think it may be useful to the OSS project,
then I will contribute back with the understanding that it may not be merged.

------
sytse
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/CONTRIBU...](https://gitlab.com/gitlab-org/gitlab-
ce/blob/master/CONTRIBUTING.md#definition-of-done)

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

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

~~~
sfifs
this strategy is ok as long as you explicitly indicate in the issue you might
not get around to reviewing soon but to go ahead and fork.

I've seen too many folks who just leave PRs hanging which leads to
frustration. People will remember and think twice about contributing to
anything with your name/id in it.

~~~
maweki
I have some PRs hanging around that are just translation fixes. How the heck
can a maintainer not merge some translation fixes? For years...

About every quarter I write something like "is there something wrong with this
PR?" "can I do something else to get this merged".

Usually without reaction.

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

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

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

~~~
akkartik
Ah, I see. Makes more sense when I reread it now. I'm just not juggling
multiple patches at a time like this, but it's a valid workflow.

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

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

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

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

------
Marazan
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
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 :)

------
caconym_
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
Also "the bus factor is high". Should be "low", as it is 1 for most projects.

~~~
NickBusey
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
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
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)

------
saw-lau
Upvoted for introducing me to the term 'bus factor.'

[https://en.wikipedia.org/wiki/Bus_factor](https://en.wikipedia.org/wiki/Bus_factor)

