Why I close pull requests • Hacker News Board

Re: Why I close pull requests

#141

Post by radarsat1 » Thu, Dec 29, 2016, 10:17 AM UTC

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.

Re: Why I close pull requests

#142

Post by maweki » Thu, Dec 29, 2016, 10:22 AM UTC

sfifs wrote: post #121

fourthark wrote: post #27

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…

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.

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.

Re: Why I close pull requests

#143

Post by pm215 » Thu, Dec 29, 2016, 10:25 AM UTC

palunon wrote: post #138

unixhero wrote: post #78

Earlier quoted context omitted.

Yeah. But it is not the way PR and Git is intended

Git is intended to work over email. How do you close an email PR ?

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)

Re: Why I close pull requests

#144

Post by saw-lau » Thu, Dec 29, 2016, 11:24 AM UTC

Upvoted for introducing me to the term 'bus factor.'

https://en.wikipedia.org/wiki/Bus_factor

Re: Why I close pull requests

#145

Post by euyyn » Thu, Dec 29, 2016, 12:39 PM UTC

fdsfsaa wrote: post #47

yegle wrote: post #13

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…

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…

I agree with what you're saying, but what you're saying is not how it works at Google. At least the teams I've been at. Design docs aren't a prerequisite to start working, and in many cases a design is nonsensical if you haven't actually at least prototyped what you want. It's just a tool to help you be comprehensive when you want to decide between alternatives, and let other eyeballs help you decide.

"this will never work", without an actual argument, isn't Googley :)

A big design doc is a liability for sure, in the same way as code is.

Re: Why I close pull requests

#146

Post by euyyn » Thu, Dec 29, 2016, 12:52 PM UTC

Sacho wrote: post #103

skj wrote: post #57

Earlier quoted context omitted.

We write design docs at Google to communicate ideas with each other. They're useful for promo committees because effective communication between engineers is something that is prerequisite for effective engineering. Of course a full design doc is not needed for every change. Many changes are small and straightforward. Only big things, where the team needs to discuss and understand options. Or bigger things, where dir…

How do you write design docs? Did you have to go through a course establishing some fundamentals to it? Did you take a writing class? Do you use specific tools? I would like to try and adopt this style of communication but everyone in my team, including me, are writing illiterates and I wouldn't even know where to start.

We don't really have a class: Like any writing, you just need to be aware of what's your goal. For a design doc, you want to say three things: (1) what are yo doing/planning to do, (2) why, and (3) what alternatives you've considered and why do you think they're worse.

We just use Google Docs, and it's easy to share the doc and have people add comments or propose changes. There's a template somewhere, but I don't like it much.

Re: Why I close pull requests

#147

Post by sytse » Thu, Dec 29, 2016, 1:28 PM UTC

geerlingguy wrote: post #42

sytse wrote: post #10

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

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.

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.

Re: Why I close pull requests

#148

Post by drubio » Thu, Dec 29, 2016, 1:35 PM UTC

yegle wrote: post #13

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…

Google isn't an engineering paragon anymore, it's too large.

I speak from what I've seen in how they handle their development & releases for Angular 2, which is a fairly large project with an equally large community.

Take a look at this release candidate, which is in between other release candidate RC.5. It has over 100+ breaking changes with new features

https://github.com/angular/angular/blob/master/CHANGELOG.md#...

This is practically against against all definitions of what a release candidate should be: https://en.wikipedia.org/wiki/Software_release_life_cycle#Re... .

So this neatly painted blanket statement of 'at google' we do x,y,z for development, new features & reviewers isn't true. There are very messy projects & development practices, practically like at any other large company.

Re: Why I close pull requests

#149

Post by digi_owl » Thu, Dec 29, 2016, 1:51 PM UTC

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.

Re: Why I close pull requests

#150

Post by Klathmon » Thu, Dec 29, 2016, 2:14 PM UTC

geerlingguy wrote: post #38

StavrosK wrote: post #29

Earlier quoted context omitted.

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…

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.

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.