DEV Community

Adewale Abati
Adewale Abati

Posted on

What are the best practices for accepting PRs?

I started contributing to open source about a year ago, and I've started a couple of projects myself so it's kind of weird I'm having this thought now.

I recently started a project and I expect to receive PRs to the dev branch. But what are the best practices to ensure that I'm not merging code that doesn't do what it's supposed to do (I don't mean broken code this time cos most can be handled with testing) into the same stream everyone is working on which is dev.

Which leads to my question or thought, whats actually the best approach to accepting PRs to a project or how do you currently do it? Because dev automatically becomes where everyone's changes end up before any release or something like that.

Top comments (5)

Collapse
 
dmfay profile image
Dian Fay • Edited

Two essential things:

  1. Automate your unit/integration tests. Your options for testing frameworks depend mostly on what language you're using. With JavaScript, you could use Mocha, AVA, Jest, tape, or a number of others. When you change code, update existing tests and add new ones to cover behavioral changes. Require the same of pull requests in your contributing guidelines. Having a robust battery of tests lets you validate code with a single command, and you can evaluate whether code does what it's supposed to by reviewing the tests.
  2. Some form of continuous integration. Most of the newer generation of CI services are easy to integrate with GitHub and other repository hosts; GitLab has its own. CI should run on every pull request, testing the merge and running your automated tests so you know immediately if a pull request breaks tests.

Beyond that, you can automate test coverage reporting with something like Coveralls. If a pull request drops coverage substantially, that's a sign that it contains functionality which isn't being tested and should be considered unpredictable.

Collapse
 
phillie profile image
Philly

YESSS to what Dian said before. 😊

Besides the technical aspects: as a maintainer of an open source project you might end up coding less but instead responding to issues, pull requests (...) more - depending on your projects popularity.

So it'd be beneficial to reduce the volume of unwanted contributions in the first place. While this may feel unkind at the beginning, it's actually helpful for both parties. To do so you should clearly communicate your expectations and explain your projects's process for submitting and accepting contributions in your contributing guidelines. Writing things down is one of the most important things you can do as a maintainer.

Also, maybe these open source guides are interesting to you?! They provide some further useful and valuable tips for open source project maintainers, and contributors. 🙂

Collapse
 
dmfay profile image
Dian Fay

Contributing guidelines are a huge asset! They help people interested in submitting pull requests figure out what they need to do without interrupting you, and if someone submits a PR that doesn't fulfill your requirements (code standards, tests, documentation) you can just point them there. GitHub recommends a CONTRIBUTING.md in your repo root which seems like a sensible standard.

Collapse
 
evanplaice profile image
Evan Plaice • Edited

Consider Scope

Ask yourself honestly.

"Is it an acceptable addition, does it fit within the project's intended functionality?"

It's OK to reject a PR if it goes in an unwanted direction. OSS projects can suffer from feature creep too if you're not intentional about acxepting contributions.

Review the code

Like review every detail until you understand it fully. Itntakes practice to get good/fast at this but it's a necessary skill for a maintainer.

Document/automate away points of friction

PRs introducing regressions, add tests and require PRs to include tests.

PRs keep introducing stylistic changes that add no value, add linting as a required step. This can waste a LOT of time, especially with inexperienced contributors.

You say dev is done on 'dev', so you follow a specific workflow (ex GitFlow). Document that in CONTRIBUTING.md. While you're at it, document your entire development process in a step-by-step manner that users will understand.

Automate Automate Automate

CI is also a good way to automate all these checks. So every PR pushed to runs through a full suite of checks (ie test, lint, etc).

The more of this you take care of early on, the less effort you'll have to waste later dealing with poor quality contributions.

Collapse
 
maestromac profile image
Mac Siri

If time permits, you should tinker around with a PR locally before you make a decision. You can always ask the contributor is clarify what their PR will do and let them know that while their PR is accepted, you will make additional changes to it to fit your need.