Skip to content
This repository has been archived by the owner on Sep 30, 2021. It is now read-only.

Commit

Permalink
DevKit updates (#401)
Browse files Browse the repository at this point in the history
  • Loading branch information
SonataCI authored Apr 28, 2017
2 parents 6da6436 + a80bc34 commit f29e206
Showing 1 changed file with 85 additions and 1 deletion.
86 changes: 85 additions & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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

0 comments on commit f29e206

Please sign in to comment.