-
Notifications
You must be signed in to change notification settings - Fork 23
Working With Code
We ❤️ code. The aim of this document is to give you guidance into why and how we work with code on a day-to-day basis.
As developers, we strive for perfect, bug-free code. But alas, there is no such thing as perfect. Bugs can and do appear in code all the time – no matter how good we think we are or what sophisticated tools we use. So if we can’t entirely prevent bugs from happening what can we do?
We implement a process of Code Review to help address this problem. Simply speaking, code reviews bring in a colleague (or a few) to give you more eyes on your work.
At a high level, the workflow goes something like this (assuming a general understanding of how git and GitHub works)…
- Select an issue to tackle and assign it to yourself in GitHub (unless someone has assigned you already).
- In your local development repository, create a new branch (see tips for naming conventions below).
- Commit your changes for the issue to that branch.
- Create a pull request to signal your code is ready for review.
- Please use the template as a guide to help describe the PR, the changes, and the steps to test it.
- Assign any appropriate labels (incl.
needs review). - Assign the pull request to yourself
- Request a review from the
woocommerce/pulsar
- A team member (or a few) will leave a review – providing feedback, questions and concerns to be addressed in comments on the pull request.
- You should then make any fixes and address any comments prior to the reviewer reviewing once again.
- Once you made the changes, leave a comment mentioning the reviewer to let them know about your changes, and re-request a review from them.
- The previous three steps then repeat as necessary to address all feedback, questions and concerns.
- Once reviewers are satisfied and the PR is “approved” (using the github functionality – the PR author can merge the pull request into the desired destination branch.
- These changes will then be shipped within the relevant release 🚀.
Work on every new feature or bug fix always starts with the creation of a new branch.
Why use branches?
- it separates work in progress from your stable/released code
- it keeps unfinished and/or experimental changes away from your stable/released code
- it keeps team members from stepping on each other's toes as multiple features/bugs can be worked in parallel without conflicting (too much) with others
- it provides a specific something to review and works well with the pull request process
If working on a specific issue the first thing to do is assign the issue to yourself so that other team members don’t double up on effort (unless someone has assigned you already).
New branches should be created with main as the base branch.
When naming a branch, it’s important to ensure the branch is clearly understandable by everyone on the team, as well as others outside of the team who may need to scan the branch list to quickly find the one branch they’re looking for.
Where possible we use the following structure :
(add/fix/update/remove)/(issue-number)-short-slug
For example
fix/1122-incorrect-order-status
The beginning/prefix identifies what is being done (e.g. adding, fixing, updating, or removing something) or identifies the type of thing being worked on (e.g. feature, release, hotfix, etc.).
After the prefix the issue number can be added – this is useful for those who prefer to identify pull requests using the issue number as it helps with scanning the list quickly.
Lastly, you can add a short slug (WordPress style) – this is useful for those who don’t remember what every issue number means. Something that’s 2-3 words long that is sufficiently descriptive and unique so as to not cause confusion is fine. Remember, it also may need to be typed in, from time to time, so consider that when writing this. Don’t spend too long thinking of the perfect slug. If it means something to you and to others on the project, that’s good enough.
Everyone works differently and has their own coding workflows so instead of being prescriptive about how you should write code here are some guidelines:
- Become familiar with and follow the WordPress Coding Standards – we run automated tests of our code against these standards
- Use atomic commits when possible – we understand this is not always possible but it makes code reviews, tracing the history, and rolling back changes easier.
- Provide clear comments for your commits describing the changes made – this helps greatly with the review process.
- Actually test your against the coding standards and any unit tests before submitting a pull request.
When your code is ready to be reviewed push the branch to Github and create a pull request.
Make sure you are NOT pushing to main.
Once the pull request has been submitted it will appear on the pull request tab of the repository and if slack notifications have been configured the relevant slack channel will be notified.
Pull Requests can take on many shapes and sizes depending on the issue/feature being addressed. We try not to be too prescriptive.
Some key points to remember when crafting your Pull Request include:
- Provide good context and background – use links, examples, screenshots, list steps to replicate. Anything that helps clearly communicate the issue, idea, concept, feature etc.
- If the pull request is to address a specific issue be sure to link to that issue (typing # with the number will auto-link the issue). Similarly, if the pull request is related to a few issues – link to those as well.
- You can also close issues via commit messages or pull request comments by using a special keyword syntax like
fixes #[github issue number]somewhere within the pull request comment text. - If your branch is fixing a bug, include steps to reproduce the bug (if not included an issue already) and any details that are necessary to verify the fix.
- @mention individuals that you specifically want to involve in the discussion, and mention why. (“/cc @findingsimple for clarification on this logic”)
- It’s OK to be explicit about what feedback you want, if any. For example, you may specifically want a discussion on the technical approach, critique on design, a review of copy.
- It’s OK to be explicit about when you want/need feedback. If the Pull Request is a work in progress or if it is urgent, please say so. Also please feel free to mention this in slack – especially for critical fixes.
- Add relevant labels and assign an appropriate milestone to identify importance e.g. critical, high, low priority. These will typically align with any that may have already been added to corresponding issues.
- If the issue is relevant to a support ticket it is useful to also include a link to the relevant support ticket.
Please use the template to shape your comment – reference the issue you are fixing, describe the PR, the changes, and the steps to test it.
Try to keep the code to be reviewed to focus on a single issue.
If there are multiple issues being reviewed in the one pull request, or there is a lot of code to review, there is a much higher chance of issues being missed by the reviewer.
It also makes the review process more time-intensive, which can lead to the pull request remaining in the review queue for weeks or months.
To begin it is often useful to do a quick check for:
- merge conflicts
- automated test failures (unit test failures, coding standard failures, e2e test failures, etc.)
If there are any merge conflicts or failures these are the first things you should let the author know about.
Next, you’ll want to read up on the pull request, any relevant issues, and any supplementary information provided. It is important to have a good understanding of the relevant issue or feature.
From here you should review the changes (the Github code preview tool is great for this) and test the code out to make sure it works as intended.
We understand everyone will develop their own individual technique and process for reviewing (based on experience etc.) but as a guideline, we have included a basic checklist below.
- Do you have enough information and understand what the issue or feature is about?
- Can you replicate the described issue/bug?
- Does the code work? Does it perform its intended function, is the logic correct, does it fix the bug etc.
- Is all the code easily understood?
- Does the code conform to WordPress coding standards.
- Is there any redundant or duplicate code?
- Is the code as modular as possible?
- Is there any commented-out code?
- When testing the code are any errors or notices triggered?
- Is there any stray logging or debugging code?
- Do comments exist and describe the intent of the code?
- Is the code testable?
- Do tests exist for this code already and does the code pass them? Do the tests need to be updated?
This is not an exhaustive, “must follow” checklist – it is intended as a guideline.
As a reviewer, if you do find any issues or have questions/feedback about a pull request you should leave comments on the relevant pull request.
You also have the ability to add inline comments to specific lines of the code. These are useful for authors as they can see the exact spot in the code you are referring to. If your comments relate to a specific piece of code rather than a broader design decision, use inline comments.
Similar to when authoring pull requests – it is important to provide as much context to your feedback as possible. You too can use links, examples, screenshots, etc. to demonstrate your findings. Try to provide specific, contextual examples to help the author understand the issue instead of generalized comments. For example, “Using this function here will mean Y is a string when we want an int later on for the call to x()” is better than “This function isn’t good because of the return type“.
Most important of all – be positive, polite and constructive. Focus feedback also on the work or issue.
It’s good to critique work, it’s never OK to critique a person.
An example of good feedback: “The logic here is backwards, it should be reversed because we want X not Y, otherwise N will happen“. This comment is focused on the work, suggests an alternative and explains why to help the author understand the problem.
An example of bad feedback: “What were you thinking here!? The logic is backwards! This is a stupid mistake“. This comment expresses needless outrage, and focuses criticism on the author, instead of providing anything constructive to improve the code.
When you submit your feedback use the corresponding type. GitHub lets you choose from: comment, request changes, approve.
When you provide multiple comments with one GH review, especially for “request changes” or “comment” try to shortly denote the “kind” of each individual comment, or problem you find.
Try to deliver your position clearly, so the PR author would know should they proceed and merge, look for a better solution, keep on explaining, or what to focus on, how to prioritize the work, etc.
As an author, if you do receive feedback on one of your pull requests do not take it as a failure or even mistake. These reviews help us constantly improve our code and ourselves as developers.
Take the time to understand what the reviewer has identified and then update the relevant branch (pushing new commits to Github) and/or respond with your own comments to address the feedback.
Constructive discussion around an issue is commonplace – it’s good to have back and forth. At the end of the day it’s all about coming up with the best solution/code for our customers. Which can be hard, but is much more likely with collaboration.
If everything is good it is time to ship!
This is as simple as merging the pull request.
When clicking the merge pull request button, you’ll be prompted for a merge commit message. We almost always use the default message as it helps us track the history of our code.
Once merged, as a rule of thumb we delete the branch that has been merged. We’ll often set up an automated bot to do this. This keeps the list of branches on our repositories tidy and we can always restore it if we find any problems down the track.
- No code is too short or too simple.
- Be positive, polite and constructive (authors and reviewers).
- It’s good to critique work, it’s never OK to critique a person.
- You don’t have to find an issue with every review. It is ok to just say “LGTM 👍” if it is actually all good.
- Provide context – use links, examples, screenshots, list steps to replicate. Anything that helps clearly communicate an issue, idea, concept etc.
- Try to keep the code (to be reviewed) to a manageable length. If there is too much to review it can make the review process very difficult.
WooCommerce is an open source eCommerce platform built for WordPress and lovingly crafted by Automattic and the WooCommerce community ❤