Ask HN: How can I make your life easier when reviewing a PR?
If you were assigned a PR, what could the submitter do to make your life easier and reviewing process less painful?
- Follow the coding conventions established for the project.
- Code quality: factor your code well, use descriptive names, use language-appropriate idioms, etc. The code needs to be easy to understand.
- Make sure you have comments to explain non-obvious things in your code.
- If the project is cross-platform, make sure your code is not dependent on a specific operating system or compiler. If it's web code, make sure it works on browsers other than the one you usually use.
- If it's a new feature (e.g., a new API method), provide documentation for it in the format used by the project.
- Don't gratuitously introduce new dependencies into the project. If the libraries being used have the functionality you need, don't add another library just because you like it better. And don't introduce an entire library just to avoid writing a few lines of code.
- If the reviewer gives you constructive criticism and asks you to re-write your code before they accept it, don't complain about it.
- Perhaps most important: If you have any questions or doubts about what you're doing, ask before submitting the code. It could save both you and the reviewer a lot of wasted effort.
Some highlights:
- Do submit one pull request to address one issue.
- Do submit two pull request to address two issues.
- Do not tack on a minor whitespace or semicolon changes in unrelated commits or Pull Requests, even if the minor change is correct. Make a dedicated commit or Pull Request instead.
In general, the smaller and easier to understand a PR is, the easier to merge. I've seen a lot of PRs lose their way due to a "while I'm here" mentality of fixing code style etc, and it often obscures the real purpose of the contribution.
Second this point in particular. I work in a small team (4 people) and we used to have a policy of doing code cleanup as we go, but we didn't do very thorough code reviews. After we improved our review policy, now using Pull Requests in Visual Studio Team Services, reviews involving code cleanup became a nightmare - hundreds of syntactic-but-not-semantic changes to dozens of files with a handful of the real changes hidden in there somewhere. We've since changed our cleanup policy so that they must be submitted separately, and even then we try to keep re-architecture and other semantic changes separate from more trivial cleanup, like typos in comments or identifiers, rearranging of methods, etc.
Not sure whether I'm not part of the main audience or the acronym is not as common as you think.