diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 659eca50..aaf4b108 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -2,11 +2,13 @@ Thanks for your interest in Sonata projects! +This document is about issues and pull requests. + ## Summary * [Issues](#issues) * [Pull Requests](#pull-requests) -* [Label rules]() +* [Code Reviews](#code-reviews) ## Issues @@ -338,6 +340,88 @@ We agreed that blank color is boring and so deja vu. Pink is the new way to do. ``` (Obviously, this commit is fake. :wink:) +## Code Reviews + +Grooming a PR until it is ready to get merged is a contribution by itself. +Indeed, why contribute a PR if there are hundreds of PRs already waiting to get reviewed and hopefully, merged? +By taking up this task, you will try to speed up this process by making sure the merge can be made with peace of mind. + +### Commenting on a PR + +Before doing anything refrain to dive head-first in the details of the PR and try to see the big picture, +to understand the subject of the PR. If the PR fixes an issue, read the issue first. +This is to avoid the pain of making a reviewer rework their whole PR and then not merging it. + +Things to hunt for : + +- missing docs . This is what you should look for first. If you think the PR lacks docs, +ask for them, because you will be better at reviewing it if you understand it better, +and docs help a lot with that. +- missing tests : Encourage people to do TDD by making clear a PR will not get merged +if it lacks tests. Not everything is easy to test though, keep that in mind. +- BC breaks : If there is a BC-break, ask for a deprecation system to be created instead, +and make sure the `master` branch is used. +- Unclear pieces of code : does the code use clear, appropriate variable or class names, +or does it use names like `data`, `result`, `WhateverManager`, `SomethingService`? +Are exception names still meaningful if you remove the `Exception` suffix? Do all +exceptions have a custom message? +Is the contributor trying to be clever or to be clear? +- Violations of the [SOLID][solid] principles : + - S : If a class is 3000 lines long, maybe it does too many things? + - O : Is there a big switch statement that might grow in the future? + - L : Does the program behave reasonably when replacing a class with a child class? + - I : Are interfaces small and easy to implement? If not, can they be split into smaller interfaces? + - D : Is the name of a class hardcoded in another class, with the `new` keyword or a static call? +- Spelling / grammar mistakes, including in commit messages or UPGRADE / CHANGELOG notes. +- Dependency modifications : is anything new introduced, if yes is it worth it? + +[solid]: https://en.wikipedia.org/wiki/SOLID_(object-oriented_design) + +Leave no stone unturned. When in doubt, ask for a clarification. If the +clarification seems useful, and does not appear in a code comment or in a commit +message, say so and / or make use a squash-merge to customize the commit message. +Ideally, the project history should be understandable without an internet connection, +and the PR should be understandable without having a look at the changes. + +Also, make sure your feedback is actionable, it is important to keep the ball rolling, +so if you raise a question, try to also provide solutions. + +### Labelling the PR + +Applying labels requires write access to PRs, but you can still advise if you do not have them. +There are several labels that will help determine what the next version number will be. +Apply the first label that matches one of this conditions, in that order: + +- `major`: there is a BC-break. The PR should target the `master` branch. +- `minor`: there is a backwards-compatible change in the API. The PR should target the stable branch. +- `patch`: this fixes an issue (not necessarily reported). The PR should target the stable branch. +- `docs`: this PR is solely about the docs. `pedantic` is implied. +- `pedantic`: this change does not warrant a release. + +Also if you see that the PR lacks documentation, tests, a changelog note, +or an upgrade note, use the appropriate label. + +### Reviewing PRs with several commits + +If there are several commits in a PR, make sure you review it commit by commit, +so that you can check the commit messages, and make sure the commit are independent +and atomic. + +### Merging + +Do not merge something you wrote yourself. Do not merge a PR you reviewed alone, +instead, merge PRs that have already be reviewed and approved by another reviewer. +If there is only one commit in the PR, prefer the squash feature, otherwise, always +use a regular merge. +And finally, use your common sense : if you see a PR about a typo, +or if there is a situation (faulty commit, revert needed) maybe you can merge it directly. + +### Be nice to the contributor + +Thank them for contributing. Encourage them if you feel this is going to be long. +In short, try to make them want to contribute again. If they are stuck, try to provide them with +code yourself, or ping someone who can help. + [sphinx_install]: http://www.sphinx-doc.org/en/stable/ [pip_install]: https://pip.pypa.io/en/stable/installing/ [sf_docs_standards]: https://symfony.com/doc/current/contributing/documentation/standards.html