All PRs need to be reviewed before merging.
Everyone is welcome to review and the more reviews the better. And even if you don’t know the affected parts of the system, your feedback on concepts, new API, documentation, configuration is very helpful!
We use the GitHub PR approval feature, so just get going and approve/request changes/comment as you see fit (and are allowed to).
Suggested workflow:
- quickly check the assigned labels, add/fix if needed (see Creating a pull request)
- start with a conceptual review (unless it’s a trivia bugfix, of course) to sort out fundamental issues first:
- assess if a new feature makes sense and fits the overall “feeling” of the system
- see if any new settings make sense and are intuitive to use
- check new/changed API being introduced for consistency with the existing code
- then go into the code in more detail:
- if there is a fundamental issue with the PR, sort that out first!
- then nitpicks can be done, if possible, fix nitpicks yourself, instead of just commenting on them
- Yes, we care for wording in method and variable names; we require proper code documentation; but don’t overdo it - discouraging a potential contributor on the first PR is probably not worth it.
Some tips:
- be constructive and honest, point out things that are good to keep motivation up
- make sure you follow up on the PR you reviewed, answer further questions
- try to get the author involved if needed
- if a PR has the “discussion” label, check and try to participate
- if needed, ask the author for push access to the fork, so you can fix them easily (or do a PR on the PR; cumbersome, but works)
(This started at Code review process for pull requests)