
The Art of Closing - david90
https://blog.jessfraz.com/post/the-art-of-closing/
======
waffle_ss
From the opposite point of view, before writing a significant patch/feature,
it may be worth opening an issue with a quick description of what you'd like
to implement to see if there's interest in it from the maintainers before
investing the time in writing it. It may even be something someone is already
working on in a private branch.

Even if it's something you really need and plan on maintaining your own fork
if rejected, you could still get some tips on how to best implement it or
potential tricky bits to be aware of.

------
wueiued
I have a policy to accept anything what is not a total crap. It is easier to
edit and fix code, than educate random people. And I might remove that patch
before stable release anyway.

Granting commit access is different level.

------
geerlingguy
As a maintainer of dozens of fairly small OSS projects on GitHub, I learned
early the importance of 'No'. Also, unmentioned here, you have to be willing
to ignore the 'me too' present in a lot of PRs and feature requests; what's
most important to me is 'will this new code/feature make the project easier or
harder to maintain?'

If harder, it had better be a stunning new addition that makes the extra
maintenance worth it... And often, it had better include an automated test!

~~~
btown
Asking the author of a PR to include an automated test is a favor to the
larger community - you're building momentum for this practice. As a
maintainer, you shouldn't feel bad about introducing this sort of hurdle. (And
if you don't have the infrastructure to run those automated tests, then it's a
good excuse to get it up and running!)

~~~
haikuginger
> Asking the author of a PR to include an automated test is a favor to the
> larger community

Agreed. Once this practice is in place, it's much easier to keep going - we
have 100% coverage on urllib3 and require any new contributions to meet that
bar.

------
sitkack
The pain of having a patch rejected is because of the asymmetry. No is _easy_,
the submitter often put hours of work into understanding the codebase, the
architecture the style of the code, figure out how to fix a bug or add a
feature and then to have the response be, "No", feels like a pretty harsh
rejection.

How we structure work on these projects needs to be rethought so that the
majority of these don't happen. The attitude of code-or-gtfo is kinda broken
with respect to how much work people put into a project with an uncertain
outcome.

Maybe we should

    
    
      * submit an issue outlining new feature, or technique to fix bug
      * create a branch, reference that issue
      * update issue for a branch review, get greenlight
      * do possibly hours worth of work
      * submit PR that isn't outright rejected
    

Just communicating through pull requests seems very macho and wasteful to me.

~~~
cyphar
> No is _easy_, the submitter often put hours of work into understanding the
> codebase [...]

And maintainers will put many, many more hours into maintaining it. The
submitter only has to interact with us once. We have to interact with their
code for a long time.

> How we structure work on these projects needs to be rethought [...]

Most projects work the way you described (including Docker). You open an issue
(and/or send a mail to the mailing list) describing the problem and a proposal
to fix it. We then discuss the design and once the maintainers all agree with
the design (more or less) you move onto creating a PR.

Sometimes maintainers won't agree, and it'll take writing a PR to convince
them that it will work (this does happen). But in most cases, the design is
the important part (if it's a non-trivial change).

The only counter-example I can think of is the Linux kernel. But that's an
extreme example and usually a dummy PR will be enough to convince them to
discuss your idea.

------
pnathan
Down with the lowest tier of projects, I generally will always gratefully
accept any help, since it doesn't come around very often. If the project is
already pretty solid on tests and docs, I usually ask for them in the patch.
Otherwise, I will do the grunt work there myself if need be.

------
hiou
Docker is the first project I can remember using that has actually
underdelivered features to the point I'm using it less and less.

I would definitely not follow their model of open source development as they
are an example of the opposite and equally ineffective extreme.

~~~
tonyhb
Docker is a really complex open source project to build. Feature wise, things
need to take into account compatibility with every OS it runs on
(linux/windows/bsd), all networking stacks, distributed systems, the actual
image distribution project, runC etc.

Underdelivered features at that scale is to be expected — quality is a bar
that can't be compromised.

------
erikb
It's true that there are not enough "no"s. And at the same time there are also
too many "no"s. E.g., many PRs are not a PR because one wants that change in
the code, but because one has a problem that needs solving and I thought about
helping the developers instead of letting them do all the work. If you just
close it without recognizing it in any way then the problem stays still
unfixed, and motivate people to do things you won't like, like patching your
project just in their company's environment and thereby creating a hidden
fork.

In the end I think the question is not about "yes" or "no" but about what's
the actual problem and not making the easiest choice but trying to make the
best.

------
mywittyname
I feel like some of these "examples" are a bit wishy-washy.

> Hi X, We really appreciate you taking the time to make this patch. However
> the design was not discussed prior to writing it. We do see potential in
> what you are trying to build, but we think it would be more effective as
> blah, blah, and blah. We are going to close this but would love to see you
> open a patch that takes the above direction. Thanks, this could really be an
> awesome feature!

That doesn't sound like a "no" to me. It sounds like they are tell me to
change some stuff around and resubmit, which is a type of "yes, but."

------
franciscop
An anecdote [1] about "No is temporary, Yes is forever":

4 months ago: However, please read the notes in Roadmap and new features
before continuing with this work, as I'm not yet sure whether or not they are
within the scope of Umbrella

[...]

4 months ago: I don't think it's a good idea to change it, so only Strings
will be available for after, before, etc.

3 months ago: I opened it to implement what was suggested here.

3 months ago: Okay, it was added.

[1]
[https://github.com/umbrellajs/umbrella/issues/20](https://github.com/umbrellajs/umbrella/issues/20)

------
rosenfeld
"Thanks so much for spending time on this amazing patch. We really appreciate
it. However I do not think this is something we want to add right now, because
of yadda yadda but in the future this can change. Thanks so much!"

This tells us you are a big liar even if they didn't read your post. If a
patch is amazing it should be merged, if it's not then it's obviously not
amazing. This kind of message is not useful. You should be more explicit in
explaining the reasons you are not accepting the patch. If you don't want the
feature other contributors won't even try to write it in another way before
convincing you to accept the feature. If you like the feature but didn't like
the patch this should be stated if you could be kind enough to explain why you
didn't like the patch, they (or others) could try other approaches.

Most of the times I get a PR I'm not interested in, my response is something
like this:

"I'm not accepting this PR due to [real reason here]. Please, since I value
your time and don't want it wasted, before submitting any PR, please open a
ticket to discuss it first. That way I can tell you beforehand whether a
change is desired or not and the reasons behind this decision and this would
save you some time."

This is one of the reasons I decided to stop contributing to Rails long ago.
After wasting too much with two patches that got ignored or rejected I decided
contributing to Rails didn't worth my time. I often ask about the new feature
before working on it. But their usual response was in the line "send us a PR
and we can discuss". I feel this is plain wrong because it doesn't respect the
contributor's time. They should think about the feature and decide on whether
it's desired, acceptable or not. Even though they say they need to take a look
at the code to know, this is not true. It's just laziness. And I'm not willing
to take some time I could be practicing Mandolin to spend on some changes that
have great chances of being ignored. This is a valuable wasted time.

Finally, back to the original post:

"These are just a few of the techniques we’ve used in the past. I hope if you
are a maintainer of a project they are helpful for you, but I would love to
know your tips as well."

It's hard to believe so, since the blog doesn't accept comments and the author
does not say how he would like to get this feedback. Also this is a very
selfish behavior in my opinion. I find it really frustrating when I see some
idea being brought to discussion in the wide but without a comments session.
Lots of what we learn comes from those discussions, sometimes even more than
from the original article. When you say you would like to hear the feedback,
this is selfish, because only you are going to get that feedback while other
readers might be interested as well.

------
askyourmother
You would think docker would take all the help they can get, given how bad
their current code base is. Maybe they should be "closing" their blog posts,
and let real developers step in.

~~~
acdha
You're talking about a very popular project used in production by many people
at many companies. If you're going to make a sweeping dismissal that it's
terrible and all of the developers are bad, you absolutely have to back up
your claims.

------
bunkydoo
Coffee is for closers.

