I find myself repeating a lot of the same comments in pull requests,
so I decided to put together a list of what I consider the most
important features of a good pull request. Other people will have
different feelings on some of these, but the points below are what
apply to my projects. If you have thoughts on things I've left out, or
reasons why you disagree with these points, please comment below.
Many of these points only make sense for source code, and even more
specifically for code written in Haskell. My content repos (like
this site's content)
and non-Haskell repos (do I have any of those???) would be slightly
NOTE: I'm not the maintainer of Stack, so the comments below do
not necessarily apply there. Stack has its own contribution rules, so
please don't take my personal opinions here as relevant to that
Every top-level identifier exported from a library needs to have a
Haddock comment. It's irrelevant if the identifier name is
completely self-commenting; a comment is still necessary. (These are
the comments that look like -- | Describe what the function does.)
My packages all follow PVP-style version numbers, and pull requests
should include bumps to the version number. For those unfamiliar:
PVP version numbers consistent of four components, A.B.C.D.
If your change fixes a bug without modifying the API at all,
then bump D (126.96.36.199 becomes 188.8.131.52, or 1.2.3 becomes 184.108.40.206).
If your change adds something new to the API without changing
something that already exists, bump C (220.127.116.11 becomes 1.2.4).
If you change the existing API (e.g., remove a function, change
semantics, modify a data type), bump either A or B (18.104.22.168
becomes either 1.3.0 or 2.0.0, depending on how big a change you
think this is).
By the way, I'm unlikely to include a breaking change unless
you have a really good reason. I consider backwards
compatibility really important. Consider exporting a new
function and (optionally) deprecating the old one instead.
To elaborate on motivation for the previous point: I follow a policy
of releasing new code fairly quickly in most cases, as opposed to
batching up a number of changes. In that situation, it makes sense
for a PR to include the new version number immediately. Many other
projects work differently, and do not encourage contributors to do
Also, I sometimes forget to make a new release after merging a
PR. If I do forget, don't be shy about pinging me to make a
Include a @since annotation in each new identifier, including
the new version number you just bumped to. This is absolutely
vital for users of a library to properly specify lower bounds in
their dependencies easily.
Don't include @since for un-exported identifiers.
If you are exporting a pre-existing identifier that was previously not exported, include the @since.
In other words: @since indicates when it was added to the
public API, not when it came into existence.
-- | Download a JPEG image.
-- @since 1.5.2
downloadJPEG :: MonadIO m
=> Request -- ^ URL to download from
-> m JPEG
Include a ChangeLog.md entry. If the project doesn't have a
ChangeLog.md, create one. Odds are the content you write in the
changelog can be identical to the pull request description. This
is a huge end-user convenience. Example:
* Added the `downloadJPEG` function
Slight exception to the above: if you're making a doc-only PR, don't bother
with a version bump, but instead add a ChangeLog entry with ## Unreleased
and a description of the change.
Do not include unrelated changes in your PR, it makes it difficult
to review, more likely to get delayed, and more likely to conflict
with other changes. Include separate changes in separate PRs.
Keep coding style consistent with the rest of the code. I'm not a
big stickler for coding style guidelines in general, but I do
consider it very jarring to have conflicting styles in the same
file, or even worse the same function.
Similar to the previous point: think hard before sending a pull
request to modify the style of code. Again, I'm not a big stickler
on coding style, and I consider style in general a pretty arbitrary
and unimportant aspect of the code. But that makes me even less
likely to want to spend my time reviewing and discussing changes
like whether records should be aligned. (For the record, I don't
think they should be aligned, as it makes diffs noisier than
If you have a PR for addressing a typo, making a trivial fix, or
adding a straightforward feature: just send a PR without any prior
discussion. However, if you want to make a major overhaul, change
behavior, or break API compatibility: it's worth opening an issue
first to discuss. I don't like rejecting PRs or causing people to
waste their time, so I'd rather hash out details before you spend a
lot of time on something.
Don't use partial functions. I've received lots of PRs which, for
example, use fromJust because "by analyzing the rest of the
code, you can tell it will always be a Just value and not
Nothing." That doesn't cut it for me:
I'm lazy, and I don't want to reason about the code. I want the
compiler to do it for me.
I'm stupid, and I don't trust my own reasoning.
Such logic does not withstand future refactorings of the code,
making it fragile.
There are definitely some exceptions to this rule, but odds are
pretty good your case won't be one of them :).
Added test cases are always a good thing. They also make it easier
for me to understand why you're writing this PR, and ensure I
don't break your work in the future (remember from the previous
point: I'm stupid).
When submitting PRs to libraries (as opposed to an application like
Keep compatibility with older versions of dependencies whenever
possible. I try to keep as broad a range of potential package
versions as I can to help avoid "dependency hell." (For the
record: dependency hell is not related to tooling in any way,
it's an intrinsic aspect of having dependencies.)
In some cases, if you drop compatibility with a major version of
a dependency (e.g., change from transformers >= 0.3 to
transformers >= 0.4), I may consider it a breaking change in
the library worthy of a major version bump.
Avoid adding dependencies. I personally am not of the opinion
that reducing the dependency footprint is that important, and
strongly believe that such behavior in general leads to Not
Invented Here (NIH) syndrome. However, since enough users of my
libraries do feel this way, it's easier on me if you don't
incur unnecessary dependencies.