About a month ago a developer submitted a pull request to one of my plugins. At first glance the proposed changes looked fine, meaning that the code was clean, formatted nicely, and made sense. After reviewing and thinking about it for a few minutes, however, I realized that I didn’t love the way the proposed change was laid out. The idea behind the change was good but the execution was not ready for prime time.
I responded to the developer and first thanked them for their suggestion and for taking the time to put together a pull request. Seriously, no matter how good or bad a suggestion is, the developer should always be thanked for their time. They took time out of their day to try and improve your plugin, thank them.
The changes proposed were, in my mind, unnecessary because there was already a straight forward method to do what the developer’s changes allowed one to do. It added an additional method to register error messages. In this particular case, I felt that it would simply cause inconsistency and confusion for other developers wishing to register their error messages. If a developer looked into the code and saw numerous “official” ways to do the same thing, they might wonder which was better, or they might question why there was two in the first place.
Aside from possibly causing minor confusion when a developer decided which method to use, was there any harm in having both? No. It certainly would have made this developer’s task a little easier because it fit right in with the integration they had already built. But I do not believe a “lack of harm” is a good reason to merge a pull request into your code base.
When there are dozens, or potentially even hundreds, of different developers working on a code base, it’s very easy for things to get muddled. One developer uses method X while another developer uses method Y.
The title of this post isn’t meant to imply that project managers should be hard asses and never accept proposals. It means that project managers need to be firm and stick to a consistent standard throughout the project. If a proposal doesn’t match up to those standards, it isn’t ready to be merged in.
I gave a presentation at WordCamp Columbus 2013 titled Encouraging Community Development. One of the key points in that presentation was that you (an OS project manager) should never accept contributions from other developers simply because they are contributions. It’s very easy to get into the “yay! a pull request from another developer!” mind set when you are first starting out with an open source project.
I chose not to accept this developer’s pull request (in the initially proposed form) because it didn’t quite line up with how the internal registration API was designed. Does that mean the original design schemas should never be changed? No, it means that the design schemas should remain consistent. Change is good but change should be made consistently. If you change one portion of the schema but neglect the others, you have just succeeded in creating fragmentation within your project.
Code changes should be hard to get accepted because if they are easy, it means the project lead is probably being far to relaxed with the kind of changes that are accepted.
“Hard to be accepted” doesn’t mean that all changes should go through rigorous back and forth, but it does mean that all changes should be seriously scrutinized before they are merged in. Some changes take all of 5 seconds to analyze and determine if they are fit while others take days, months, or even years.
A perfect example of a “hard to be accepted” patch that is 100% functional and makes a significant improvement to many developers is #21488 for WordPress core. It is a patch I wrote at WordCamp San Francisco nearly two years ago for adding default HTML field callbacks for the Settings API. At first it seemed like a no brainer but as more and more time passed, it became clear that it probably wouldn’t get merged in, even though it was such a simple fix. One of the reasons is due to the up and coming new Settings API that has been proposed. Why change an old API when a new one is coming in?
Whether one agrees with the reasoning to not merge a change in, the fact that a change is hard to merge in (assuming it’s not just ignored) is a good thing. It shows the the project maintainers are thinking about the big picture and are taking the future changes into consideration.