IxDF Developer Docs

Appearance

Pull Request: Creator’s guide ​

Introduction ​

Let's be real, getting your work scrutinized is hardly a good feeling for anyone. You put a lot of brain and energy into finding a solution for a difficult problem and now someone tries to find any problem you may have overlooked. Therefore it is important that we agree on how we handle reviews, understanding that every review is a learning experience with benefits for you and the reviewer.

As a matter of fact, we highly encourage you to review as much as possible of the work of your colleagues, it helps you to learn about unfamiliar parts of our system, as well as improving the overall code quality. For more information see how to review PRs yourself - Reviewers Guide

Preparation ​

Create a feature branch ​

Create a branch that references the GitHub issue nr with a short description of the task

shell
> git checkout -b 12345-improve-pr-documentation

For more info on how we handle branches and commit messages, please read our Git workflows

Zero Issue - No existing issue for your work? ​

Following our Git and contribution guidelines, you need an GitHub issue assigned for the work, but may don't have it. We use common sense to decide how to proceed.

If your changes potentially break something, please create a new issue by yourself and proceed in our normal manner. Otherwise, it can be appropriate to commit with a #0 prefix directly IF the commit message body adds enough context instead.

Do the work & write the tests ​

There are many solutions to a problem, but we strive for consistency. It is often a very good idea to research the codebase on how a similar problem has been solved before and assimilate the same style.

When you find a pattern in the codebase that solves your problem, it's probably the correct way to do it. If you have to invent a lot of new things, something is probably wrong and the new pattern will be scrutinized.

Create a Pull Request ​

Save as draft version ​

After your last feature code is pushed, create the pull request on GitHub or in your IDE.

  1. Use the PULL_REQUEST_TEMPLATE (this contains placeholder text that we use for every new pull request, please do not ignore it).
  2. Write a short and meaningful PR title using imperative sentence structure. The title should briefly describe your changes, e.g. Add Membership history feature or Fix VAT for EU companies. The title will be used for the release notes.
  3. General rule: Every PR should improve the codebase (unless it’s a hot-fix).
  4. Assign yourself as the PR owner/Assignee.
  5. if you are not 100% sure if the PR is ready, save it as draft (as a pro, feel free to skip that)

Optional: Read more about the official GitHub recommendations here: How to write the perfect pull request.

Set PR labels ​

For pr-size and pr-label, e.g. pr-size:patch and pr-category:fix. You will get notified if you forget it. Learn more about how we are use labels

Convert draft to PR ​

After doing these last updates to the draft, save it as a PR. Multiple automatic CI builds should start now, please wait until all are through.

Final inspection ​

Please read through every single file and changed line of your PR and ask yourself: "Should this be here?" This is what your teammates will do, so respect all reviewers by being your biggest critic first.

If there are non-related changes showing, you may need to rebase your feature branch until only your changes are visible.

Please make sure that all auto code-quality warnings (Rector, Psalm, CodCov, PhpStan) shown in the PR are resolved

Only when you are 100% ready, proceed with assigning reviewers

Note: There are maybe situations where it's not feasible to resolve certain conflicts. If this is the case, please outline this in the PR description

Assign reviewer(s) ​

Next you'll need to notify your teammates that you need the feedback to get your feature released.

Primary Reviewer ​

Use the Domain Modules and Code Owners document to find the system owners and assign one of them to review your PR. This person has the most domain knowledge, and you want to listen very carefully to their advice. Once they approve, you are good to merge and don't need further approval.

Note: This sometimes assigns multiple reviewers due to them being code owners. Please remove all but one of the automatically assigned code owners unless there's a specific reason that you want more than one reviewer.

Secondary Reviewers ​

We highly encourage that anyone with potential insights is requested as secondary reviewer as well. They may give very little or even no feedback, but sometimes can offer a fresh mind on certain overlooked aspects, improve phrasing and best of all: learn about the cool thing you just build. You don't need to wait until each requested reviewer replied, the code owner is the one who mostly counts.

Please use independent judgement to decide if a secondary reviewer should be invited to the review. There may be many different reasons to do so. For example:

  • A developer with previous experience working with the code might be in a good position to be a secondary reviewer.
  • If your pull request contains non-trivial front-end changes, it might be beneficial to invite a front-end developer as a secondary reviewer.

Review feedback ​

All reviewers aim to give you a quick response, supporting you in getting the feature shipped. For the most qualified code owners this is a huge burden and things may pile up, blocking yourself from moving forward.

We can help by taking responsibility, writing clear and clean PRs and give support as secondary reviewers

Comment ​

Is usually meant to ask questions or make minor points that are up to discussion. Most common for secondary reviewers, this is neither an approval nor rejection but often offers insights. It's mostly up to yourself to decide if there is something to do or not.

Approval - Yes ​

Coming mostly from the code owner, giving you the green light that you reached IxDF standard, ready to get your stuff shipped. The reviewer’s responsibility is done, the PR author should do all the rest:

Rejection - Oh no, now what? ​

A rejection simply means that your feature is not at IxDF standard, not that you or your skills are insufficient.

Read the review comments carefully and apply suggestions to improve your PR. If you need more information or do not agree with the suggestions, continue the discussion preferably in a one-to-one call. We avoid long, inefficient back-and-forth debates in the comment sections and rather clarify what was meant in person.

Once you have changed your code according to reviewer’s comments and both sides reached agreement, please give a short feedback and resolve the comment by yourself.

In the very rare case, when no agreement can be found, please feel free to reach out at our #dev channel, making your point. Your colleagues will do their best to judge fair and unbiased. Any consensus here is final, no second appeals.

Once you think all issues are resolved, please don't forget to re-request review so the reviewer gets notified.

Materials ​

  1. Optional: Pull request descriptions