9

When contributing to an open source project that uses pull requests exclusively for contributions, I'm left wondering when a good time is for me to send a pull request.

I'm especially in doubt when it come to implementing rather major features, should I implement the entire feature on my fork and only push upstream when it works? My worry here is that it might be a lot of effort for them to check if my work is correct when they have to review a huge amount of code.

On the other end of the spectrum, I'm afraid that sending a ton of tiny pull requests might get on people's nerves as well.

What should I keep in mind when trying to figure out how quickly I should send a pull request?

This will obviously vary from project to project, I'd like to know what is the general preference and what the variations tend to depend on. This might also help me set standards if I start my own project.

  • 2
    This is a rather experimental question and might either be off-topic or too broad. Don't hesitate to vote to close and comment (!) if you see problems with it. – overactor Jul 13 '15 at 11:57
  • So, a quandary. This is a great question, but probably belongs on the relevant support board for the project you are submitting to. There will be as many opinions on this as there are projects :) There is no absolute right answer unfortunately. Hence the close vote. – kdopen Jul 13 '15 at 14:04
  • @kdopen an absolute answer might note exist, but maybe some general guidelines and a very reasonable educated guess might? In either case, thanks for the input, we'll see how the community decides. – overactor Jul 13 '15 at 14:07
  • 1
    The real problem is that "one big pull request" may work for 4 projects, and then get rejected on the fifth precisely because it is one big request. Git for example prefers a series of very small changes. and of course, the opposite holds true as well. So any advice given and followed here is likely to lead to a rejection somewhere – kdopen Jul 13 '15 at 14:09
  • Keep in mind that "one pull request" doesn't have to mean "one commit". – svick Aug 1 '15 at 21:02
6

Pull requests should be easy to review and integrate. Big pull requests can be easy if done properly, and small pull requests can be extremely tricky.

If your change is going to be big, or if it has a huge impact (e.g. breaks backwards compatibility), you should discuss what you are going to do with the maintainers first. It may turn out that some of them are already working on an alternative solution, or if the upcoming changes are about to make your original idea inapplicable and it will need modifications, for instance. If the design and implementation details of your feature/fix are agreed upon, it's usually easy to review and merge. Also, it allows other people to collaborate with you on it since they also know what is going on.

On the other hand, suppose I send you a one-line fix that uses obscure syntax and I don't supply any explanation of what precisely it does. It's probably easier to dismiss than to find out if it's worth merging.

I think one pull request should correspond exactly to one logical change though, no matter how big. Several unrelated changes such as fixes for totally different bugs or several features unrelated to one another is a bad idea, even if the pull request is small in terms of size.

6

You should only submit a pull request if it is complete. The problem is the definition of "complete".

Your changes

  • must not break any existing unit/regression/integration tests.
  • must include sample code and/or documentation for how to configure, deploy, use the feature,
  • must be based on the current commit of the branch you want your changes to be merged against,
  • should implement a feature that can actually be used OR
  • should include a complete change to parts of the system that are not directly usable but that prepare the ground for future changes.
4

The only correct answer here is "it depends".

You need to know your audience - in this case the people who will be accepting the pull requests.

Often, when you first start contributing to a project, the best approach is to choose a small (usually low priority) bug. Being low priority, it's probably not being worked on yet. Being a small fix, it is an excellent "hello world" introduction to that project's community and their practices.

Next, clone the repository ('pull request' as a mechanism is generally related with Git-like VCS systems), and study the pattern of accepted requests. Look for similar small fixes, and see how they were done.

Then FOLLOW the style which is used in that project

As an example, I have seen a bug fix in Git rejected because the developer fixed the bug and renamed a local variable in the same commit. The renaming made sense (it was too similar to another, which had led to the bug being introduced). The community requested two pull-requests: one which did nothing more than rename the variable, and another which fixed the bug. Neither was more than a few lines of code. Both were accepted.

On other projects, I have seen entire features come in as a single commit. This makes sense when all changes are additive (perhaps adding a new entry in a dispatch table, and the functions that the entry calls to add the feature.

Bear in mind that there is a difference between a "pull request" and a commit. Your single pull request can contain multiple commits.

2

It depends on what changes you're making. I judge my submission of pull requests by criticality: for critical issues, it doesn't really matter how much you change, if you're fixing the issue; for tiny issues like code style, you just shouldn't change it because the author wrote it how the author wants it.

You could even assign a numeric scale to it, with 0 being uncritical and 10 being fix-this-or-die:

00: code style                      Don't change
...
05: UI design errors                One line probably won't suffice
...
10: critical security flaws         Whatever's necessary

In general, though, if you're fixing an issue you should submit a request that contains a complete fix of the issue, having previously forked the repository and made and tested your changes. Whatever you do, it should be complete: a complete new feature, a complete fix, a complete page of documentation, etc. This applies even for tiny fixes: make them complete, and you can submit a pull request. The author may like you more if you bundle lots of little issue fixes, but if you've only got one to fix, submit when complete.

  • What about tiny fixes? Separate pull requests or bundle them up? – overactor Jul 13 '15 at 12:01
  • @overactor see edits – ArtOfCode Jul 13 '15 at 12:04
  • Would it be fair to say though that when I fix 10 tiny issues in one day, I shouldn't send 10 pull requests that day? – overactor Jul 13 '15 at 12:06
  • 2
    Yes, if you've got more than one issue to fix, bundle them - but don't wait until you have more issues just so you can bundle if you don't have any. – ArtOfCode Jul 13 '15 at 12:10
  • @ArtOfCode Many projects will very much want you to submit each fix as a separate (and hopefully independent) commit – kdopen Jul 13 '15 at 14:22
2

If you submit a pull request that's too big, they will just reject it and tell you to split it up. If you submit one that's too small, they will just reject it and tell you to keep the branch around until it's done. Pull requests are public. You can get a good feel for the preferences of particular projects by viewing previous pull requests.

In my experience, most first-time contributors tend to err on the side of pull requests being a little too small. They are eager to get their change accepted, and in the rush, they skip things like documentation, style guidelines, and unit tests.

Larger endeavors should be run by the maintainers first. Again, in my experience, this doesn't tend to be an issue, as most contributors are eager (some say over-eager) to tell people about their idea. I did this successfully myself as a new contributor twice. In both instances I was asked to make one small pull request with a proof-of-concept. Once they saw my idea worked and how it fit into the project, I was given the go ahead to implement it more extensively, in a second, much larger, pull request.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.