Appearance
Pull Request: Reviewer’s guide
Introduction
We see reviews as learning opportunities for PR authors as well as reviewers. They are important to:
- share domain knowledge
- share technical knowledge
- ensuring code quality
- identify missing documentation or test coverage
Role: PR author
It is the responsibility of the PR author to ensure that the functionality is implemented in a correct way, the code meets security standards and site performance is not affected in a negative way. Furthermore, it is the PR author's responsibility to take care of test coverage, improve documentation and assimilate the established IxDF Coding Standard.
Role: Primary Reviewer and Code Owner
Code Owners are the primary reviewers of code changes in their domain. You have the deepest understanding in risk mitigation and are able to judge if changes could negatively impact our members.
Review the pull request thoroughly. When you are done, please give your feedback according to the guidelines given in the creators guide:
- your Approval is the only requirement before a merge, don't give it lightly
- a Rejection should be always applied when the as-is state of a PR is not safe.
Responsibility
Make sure there is no negative impact for our members and our IxDF Coding Standard is met.
Role: Secondary Reviewers
To support your colleagues, please proactively seek opportunities as secondary reviewer
This new role is aimed to involve a wider variety of team members into the review process with the main motivations:
- take review workload off primary reviewers
- learn about unfamiliar domains, spreading knowledge
- outside perspective; a fresh pair of eyes often asks overlooked questions
- getting PRs faster shipped
Responsibility
A partial review according to your expertise.
- Frontend developers can check for the correct usage of HTML in Blade templates, valid CSS selectors and if images are properly optimized, all according to our standards.
- Check text phrasing, could we express something more clearly?
- Coding Standard violations for naming, location or architectural approach
- Requesting clarification on the PRs intent or unclear evaluation of different approaches.
Once you are done with your review, please give feedback in the form of a Comment
Time management and notifications
No matter which role you fill in our review process, please:
- Review the PR not later than 24 hours after assignment. Ideally, pull request reviews should be the first thing you work on at the beginning of each day. If you are not able to review it within 24 hours, contact the author and make a decision on whether to delay the review wait more or delegate it to someone else.
- Set up your GitHub account to receive and read notifications (emails and/or push notifications) from GitHub for the PR.
How to review
This article contains great advice and best practices on how to perform code review, as well as having your code reviewed: Code Review Guidelines
Best practices
- Be kind.
- Accept that many programming decisions are opinions. Discuss tradeoffs, which you prefer, and reach a resolution quickly.
- Ask questions; don’t make demands. (“What do you think about naming this?”)
- Ask for clarification. (“I don’t understand. Can you clarify?”)
- Be explicit. Remember, people don’t always understand your intentions online.
- Be humble. (“I’m not sure—let’s look it up.”)
- Consider one-on-one chats or video calls if there are too many “I don’t understand” or “Alternative solution:” comments. Post a follow-up comment summarizing the one-on-one discussion.
- Use conventional: comments