From b91191a7348faaea219f79383b50911fd812ee88 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Wed, 1 Jul 2015 17:27:21 +0200 Subject: [PATCH 1/8] Added initial draft of "Community Reviews" page --- contributing/community/index.rst | 1 + contributing/community/reviews.rst | 189 +++++++++++++++++++++++++++++ 2 files changed, 190 insertions(+) create mode 100644 contributing/community/reviews.rst diff --git a/contributing/community/index.rst b/contributing/community/index.rst index 43b7b527ee3..31bbedc747c 100644 --- a/contributing/community/index.rst +++ b/contributing/community/index.rst @@ -5,4 +5,5 @@ Community :maxdepth: 2 releases + reviews other diff --git a/contributing/community/reviews.rst b/contributing/community/reviews.rst new file mode 100644 index 00000000000..3c00ebb10be --- /dev/null +++ b/contributing/community/reviews.rst @@ -0,0 +1,189 @@ +Community Reviews +================= + +Symfony is an open-source project driven by a large community. If you don't feel +ready to contribute code or patches, reviewing issues and pull requests (PRs) +can be a great start to get involved and give back. + +Why Reviewing is Important +-------------------------- + +Community reviews are essential for the development of the Symfony framework, +since there are many more pull requests and bug reports than there are members +in the Symfony core team to review, fix and merge them. + +On the `Symfony issue tracker`_, you can find many items in a "Needs review" +status: + +* **Bug Reports**: Bug reports primarily need to be checked for completeness. + Is any important information missing? Can the bug easily be reproduced? + +* **Pull Requests**: Pull requests contain code that fixes a bug or implements + new functionality. Reviews of pull requests ensure that they are implemented + properly, are covered by test cases, don't introduce new bugs and maintain + backwards compatibility. + +Note that **anyone who has some basic familiarity with Symfony and PHP can +review bug reports and pull requests**. You don't need to be an expert to help. + +Be Constructive +--------------- + +Before you begin, remember that you are looking at the result of someone else's +hard work. A good review post thanks the contributor for their work, identifies +what was done well, identifies what should be improved, and suggests a next +step. + +Create a GitHub Account +----------------------- + +Symfony uses GitHub_ to manage bug reports and pull requests. If you want to +do reviews, you need to `create a GitHub account`_ and log in. + +The Bug Report Review Process +----------------------------- + +A good way to get started with reviewing is to pick a bug report from the +`bug reports in need of review queue`_. + +The steps for the review are: + +1. **Is the Report Complete?** + + Good bug reports contain a link to a fork of the `Symfony Standard Edition`_ + (the "reproduction project") that reproduces the bug. If it doesn't, the + report should at least contain enough information and code samples to + reproduce the bug. + +2. **Reproduce the Bug** + + Download the reproduction project and test whether the bug can be reproduced + on your system. If the reporter did not provide a reproduction project, + create one by forking_ the `Symfony Standard Edition`_. Reproduce the bug + with the instructions given by the reporter. + +3. **Update the Issue Status** + + At last, add a comment and update the status of the bug report. **Thank the + reporter for reporting the bug**. Include the line ``Status: `` in + your comment to update the status to one of the following: + + **Needs Work** If the bug does not contain enough information to be + reproduced, explain what information is missing and move the report to this + status. + + **Works for me** If the bug *does* contain enough information to be + reproduced but works on your system, or if the reported bug is a feature and + not a bug, provide a short explanation and move the report to this status. + + **Reviewed** If you can reproduce the bug, move the report to this status. + If you created a reproduction project, include the link to the project in + your comment. + +.. topic:: Example + + Here is a sample comment for a bug report that could be reproduced: + + .. code-block:: text + + Thank you @weaverryan for creating this bug report! This indeed looks + like a bug. I reproduced the bug in the "kernel-bug" branch of + https://github.com/webmozart/symfony-standard. + + Status: Reviewed + +The Pull Request Review Process +------------------------------- + +The process for reviewing pull requests (PRs) is similar to doing a review of a +bug report. Reviews of pull requests usually take a little longer since you need +to understand the functionality that has been fixed or implemented and then find +out whether the implementation is complete. + +It is okay to do partial reviews. If you do a partial review, post how far you +got and leave the PR in "Needs Review" state. + +The steps for the review are: + +1. **Is the PR Complete**? + + Every pull request must contain a header that gives some basic information + about the PR. You can find the template for that header in the + `Contribution Guidelines`_. + +2. **Is the Base Branch Correct?** + + GitHub displays the branch that a PR is based on below the title of the + pull request. Is that branch correct? + + * Bugs should be fixed in the oldest, maintained version that contains the + bug. Check `Symfony's Release Schedule`_ to find the oldest currently + supported version. + + * New features should always be added to the current development version. + Check the `Symfony Roadmap`_ to find the current development version. + +2. **Reproduce the Problem** + + Read the issue that the pull request is supposed to fix. Reproduce the + problem on a clean `Symfony Standard Edition`_ project and try to understand + why it exists. + +3. **Review the Code** + + Read the code of the pull request and check it against some common criteria: + + * Does the code address the issue the PR is intended to fix/add? + * Does the PR stay within scope to address *only* that issue? + * Does the PR contain automated tests? Do those tests cover all relevant + edge cases? + * Does the PR contain sufficient comments to easily understand its code? + * Does the code break backwards compatibility? If yes, does the PR header say + so? + * Does the PR contain deprecations? If yes, does the PR header say so? Does + the code contain ``trigger_error()`` statements for all deprecated + features? + * Are all deprecations and backwards compatibility breaks documented in the + latest UPGRADE-X.X.md file? Do those explanations contain "Before"/"After" + with a clear upgrade path? + +4. **Test the Code** + + Take your project from step 2 and test whether the PR works properly. + + TODO: precise steps + +5. **Update the PR Status** + + At last, add a comment and update the status of the PR. **Thank the + contributor for working on the PR**. Include the line ``Status: `` in + your comment to update the status to one of the following: + + **Needs Work** If the PR is not yet ready to be merged, explain the issues + that you found and move it to this status. + + **Reviewed** If the PR satisfies all the checks above, move it to this + status. A core contributor will soon look at the PR and decide whether it can + be merged or needs further work. + +.. topic:: Example + + Here is a sample comment for a PR that is not yet ready for merge: + + .. code-block:: text + + Thank you @weaverryan for working on this feature! It seems that test + cases are missing for the cases that the counter is zero or smaller. + Could you add them please? + + Status: Needs Work + +.. _GitHub: https://github.com +.. _Symfony issue tracker: https://github.com/symfony/symfony/issues +.. _Symfony Standard Edition: https://github.com/symfony/symfony-standard +.. _create a GitHub account: https://help.github.com/articles/signing-up-for-a-new-github-account/ +.. _forking: https://help.github.com/articles/fork-a-repo/ +.. _bug reports in need of review queue: https://github.com/symfony/symfony/issues?utf8=%E2%9C%93&q=is%3Aopen+is%3Aissue+label%3A%22Bug%22+label%3A%22Status%3A+Needs+Review%22+ +.. _Contribution Guidelines: https://github.com/symfony/symfony/blob/master/CONTRIBUTING.md +.. _Symfony's Release Schedule: http://symfony.com/doc/current/contributing/community/releases.html#schedule +.. _Symfony Roadmap: https://symfony.com/roadmap From 65a5a9453cc59c7a9ed70577b2b8c481f7e16a5a Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Wed, 1 Jul 2015 17:29:32 +0200 Subject: [PATCH 2/8] Added link to PRs in need of review --- contributing/community/reviews.rst | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/contributing/community/reviews.rst b/contributing/community/reviews.rst index 3c00ebb10be..c0528d32097 100644 --- a/contributing/community/reviews.rst +++ b/contributing/community/reviews.rst @@ -44,7 +44,7 @@ The Bug Report Review Process ----------------------------- A good way to get started with reviewing is to pick a bug report from the -`bug reports in need of review queue`_. +`bug reports in need of review`_. The steps for the review are: @@ -103,7 +103,7 @@ out whether the implementation is complete. It is okay to do partial reviews. If you do a partial review, post how far you got and leave the PR in "Needs Review" state. -The steps for the review are: +Pick a pull request from the `PRs in need of review`_ and follow these steps: 1. **Is the PR Complete**? @@ -183,7 +183,8 @@ The steps for the review are: .. _Symfony Standard Edition: https://github.com/symfony/symfony-standard .. _create a GitHub account: https://help.github.com/articles/signing-up-for-a-new-github-account/ .. _forking: https://help.github.com/articles/fork-a-repo/ -.. _bug reports in need of review queue: https://github.com/symfony/symfony/issues?utf8=%E2%9C%93&q=is%3Aopen+is%3Aissue+label%3A%22Bug%22+label%3A%22Status%3A+Needs+Review%22+ +.. _bug reports in need of review: https://github.com/symfony/symfony/issues?utf8=%E2%9C%93&q=is%3Aopen+is%3Aissue+label%3A%22Bug%22+label%3A%22Status%3A+Needs+Review%22+ +.. _PRs in need of review: https://github.com/symfony/symfony/issues?utf8=%E2%9C%93&q=is%3Aopen+is%3Apr+label%3A%22Status%3A+Needs+Review%22+ .. _Contribution Guidelines: https://github.com/symfony/symfony/blob/master/CONTRIBUTING.md .. _Symfony's Release Schedule: http://symfony.com/doc/current/contributing/community/releases.html#schedule .. _Symfony Roadmap: https://symfony.com/roadmap From 899c782a5b84007819b6de0c03b7634481bb9a93 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Wed, 1 Jul 2015 17:38:39 +0200 Subject: [PATCH 3/8] Improved sample comment --- contributing/community/reviews.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contributing/community/reviews.rst b/contributing/community/reviews.rst index c0528d32097..4fc984d056c 100644 --- a/contributing/community/reviews.rst +++ b/contributing/community/reviews.rst @@ -172,9 +172,9 @@ Pick a pull request from the `PRs in need of review`_ and follow these steps: .. code-block:: text - Thank you @weaverryan for working on this feature! It seems that test - cases are missing for the cases that the counter is zero or smaller. - Could you add them please? + Thank you @weaverryan for working on this! It seems that your test + cases don't cover the cases when the counter is zero or smaller. + Could you add some tests for that? Status: Needs Work From d08ccf8d2cc9cd3813d36cef2694ab93cb2b12a5 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Wed, 1 Jul 2015 17:39:21 +0200 Subject: [PATCH 4/8] Fixed case --- contributing/community/reviews.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contributing/community/reviews.rst b/contributing/community/reviews.rst index 4fc984d056c..a641a033afb 100644 --- a/contributing/community/reviews.rst +++ b/contributing/community/reviews.rst @@ -12,7 +12,7 @@ Community reviews are essential for the development of the Symfony framework, since there are many more pull requests and bug reports than there are members in the Symfony core team to review, fix and merge them. -On the `Symfony issue tracker`_, you can find many items in a "Needs review" +On the `Symfony issue tracker`_, you can find many items in a "Needs Review" status: * **Bug Reports**: Bug reports primarily need to be checked for completeness. From 2d7622be96cf93725cf84ebe0342f61ad5613e34 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Thu, 2 Jul 2015 09:49:23 +0200 Subject: [PATCH 5/8] Fixed numbering --- contributing/community/reviews.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contributing/community/reviews.rst b/contributing/community/reviews.rst index a641a033afb..f046ff8dea1 100644 --- a/contributing/community/reviews.rst +++ b/contributing/community/reviews.rst @@ -123,13 +123,13 @@ Pick a pull request from the `PRs in need of review`_ and follow these steps: * New features should always be added to the current development version. Check the `Symfony Roadmap`_ to find the current development version. -2. **Reproduce the Problem** +3. **Reproduce the Problem** Read the issue that the pull request is supposed to fix. Reproduce the problem on a clean `Symfony Standard Edition`_ project and try to understand why it exists. -3. **Review the Code** +4. **Review the Code** Read the code of the pull request and check it against some common criteria: @@ -147,13 +147,13 @@ Pick a pull request from the `PRs in need of review`_ and follow these steps: latest UPGRADE-X.X.md file? Do those explanations contain "Before"/"After" with a clear upgrade path? -4. **Test the Code** +5. **Test the Code** Take your project from step 2 and test whether the PR works properly. TODO: precise steps -5. **Update the PR Status** +6. **Update the PR Status** At last, add a comment and update the status of the PR. **Thank the contributor for working on the PR**. Include the line ``Status: `` in From 62a4e85b5005f71272fa0fbe4e36470bacc9ae0b Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Thu, 2 Jul 2015 09:50:11 +0200 Subject: [PATCH 6/8] Wording --- contributing/community/reviews.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contributing/community/reviews.rst b/contributing/community/reviews.rst index f046ff8dea1..145dbcdd159 100644 --- a/contributing/community/reviews.rst +++ b/contributing/community/reviews.rst @@ -133,7 +133,7 @@ Pick a pull request from the `PRs in need of review`_ and follow these steps: Read the code of the pull request and check it against some common criteria: - * Does the code address the issue the PR is intended to fix/add? + * Does the code address the issue the PR is intended to fix/implement? * Does the PR stay within scope to address *only* that issue? * Does the PR contain automated tests? Do those tests cover all relevant edge cases? From d13a03b0e70b063bc8b270d2bd22ff9e89480503 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Mon, 20 Jul 2015 13:49:30 +0200 Subject: [PATCH 7/8] Integrated PR comments --- contributing/code/patches.rst | 2 + contributing/community/reviews.rst | 63 +++++++++++++++++------------- 2 files changed, 37 insertions(+), 28 deletions(-) diff --git a/contributing/code/patches.rst b/contributing/code/patches.rst index ade8c539a89..90d1ea2eea9 100644 --- a/contributing/code/patches.rst +++ b/contributing/code/patches.rst @@ -253,6 +253,8 @@ Check that all tests still pass and push your branch remotely: $ git push --force origin BRANCH_NAME +.. _contributing-code-pull-request: + Make a Pull Request ~~~~~~~~~~~~~~~~~~~ diff --git a/contributing/community/reviews.rst b/contributing/community/reviews.rst index 145dbcdd159..309113910d1 100644 --- a/contributing/community/reviews.rst +++ b/contributing/community/reviews.rst @@ -5,7 +5,7 @@ Symfony is an open-source project driven by a large community. If you don't feel ready to contribute code or patches, reviewing issues and pull requests (PRs) can be a great start to get involved and give back. -Why Reviewing is Important +Why Reviewing Is Important -------------------------- Community reviews are essential for the development of the Symfony framework, @@ -16,7 +16,7 @@ On the `Symfony issue tracker`_, you can find many items in a "Needs Review" status: * **Bug Reports**: Bug reports primarily need to be checked for completeness. - Is any important information missing? Can the bug easily be reproduced? + Is any important information missing? Can the bug be easily reproduced? * **Pull Requests**: Pull requests contain code that fixes a bug or implements new functionality. Reviews of pull requests ensure that they are implemented @@ -30,9 +30,9 @@ Be Constructive --------------- Before you begin, remember that you are looking at the result of someone else's -hard work. A good review post thanks the contributor for their work, identifies -what was done well, identifies what should be improved, and suggests a next -step. +hard work. A good review comment thanks the contributor for their work, +identifies what was done well, identifies what should be improved and suggests a +next step. Create a GitHub Account ----------------------- @@ -48,25 +48,27 @@ A good way to get started with reviewing is to pick a bug report from the The steps for the review are: -1. **Is the Report Complete?** +#. **Is the Report Complete?** Good bug reports contain a link to a fork of the `Symfony Standard Edition`_ (the "reproduction project") that reproduces the bug. If it doesn't, the report should at least contain enough information and code samples to reproduce the bug. -2. **Reproduce the Bug** +#. **Reproduce the Bug** Download the reproduction project and test whether the bug can be reproduced on your system. If the reporter did not provide a reproduction project, create one by forking_ the `Symfony Standard Edition`_. Reproduce the bug with the instructions given by the reporter. -3. **Update the Issue Status** +#. **Update the Issue Status** - At last, add a comment and update the status of the bug report. **Thank the - reporter for reporting the bug**. Include the line ``Status: `` in - your comment to update the status to one of the following: + At last, add a comment to the bug report. **Thank the reporter for reporting + the bug**. Include the line ``Status: `` in your comment to update + the status of the ticket. This line will trigger our `Carson Bot`_ which + updates the labels of the issue accordingly. You can set the status to one of + the following: **Needs Work** If the bug does not contain enough information to be reproduced, explain what information is missing and move the report to this @@ -100,36 +102,36 @@ bug report. Reviews of pull requests usually take a little longer since you need to understand the functionality that has been fixed or implemented and then find out whether the implementation is complete. -It is okay to do partial reviews. If you do a partial review, post how far you -got and leave the PR in "Needs Review" state. +It is okay to do partial reviews. If you do a partial review, comment how far +you got and leave the PR in "Needs Review" state. Pick a pull request from the `PRs in need of review`_ and follow these steps: -1. **Is the PR Complete**? +#. **Is the PR Complete**? Every pull request must contain a header that gives some basic information about the PR. You can find the template for that header in the - `Contribution Guidelines`_. + :ref:`Contribution Guidelines `. -2. **Is the Base Branch Correct?** +#. **Is the Base Branch Correct?** GitHub displays the branch that a PR is based on below the title of the pull request. Is that branch correct? * Bugs should be fixed in the oldest, maintained version that contains the - bug. Check `Symfony's Release Schedule`_ to find the oldest currently - supported version. + bug. Check :doc:`Symfony's Release Schedule ` to find the oldest + currently supported version. * New features should always be added to the current development version. Check the `Symfony Roadmap`_ to find the current development version. -3. **Reproduce the Problem** +#. **Reproduce the Problem** Read the issue that the pull request is supposed to fix. Reproduce the problem on a clean `Symfony Standard Edition`_ project and try to understand why it exists. -4. **Review the Code** +#. **Review the Code** Read the code of the pull request and check it against some common criteria: @@ -145,19 +147,23 @@ Pick a pull request from the `PRs in need of review`_ and follow these steps: features? * Are all deprecations and backwards compatibility breaks documented in the latest UPGRADE-X.X.md file? Do those explanations contain "Before"/"After" - with a clear upgrade path? + examples with clear upgrade instructions? -5. **Test the Code** + Eventually, some of these aspects will be checked automatically. - Take your project from step 2 and test whether the PR works properly. +#. **Test the Code** + + Take your project from step 3 and test whether the PR works properly. TODO: precise steps -6. **Update the PR Status** +#. **Update the PR Status** - At last, add a comment and update the status of the PR. **Thank the - contributor for working on the PR**. Include the line ``Status: `` in - your comment to update the status to one of the following: + At last, add a comment to the PR. **Thank the contributor for working on the + PR**. Include the line ``Status: `` in your comment to update the + status of the ticket. This line will trigger our `Carson Bot`_ which updates + the labels of the issue accordingly. You can set the status to one of the + following: **Needs Work** If the PR is not yet ready to be merged, explain the issues that you found and move it to this status. @@ -174,7 +180,7 @@ Pick a pull request from the `PRs in need of review`_ and follow these steps: Thank you @weaverryan for working on this! It seems that your test cases don't cover the cases when the counter is zero or smaller. - Could you add some tests for that? + Could you please add some tests for that? Status: Needs Work @@ -188,3 +194,4 @@ Pick a pull request from the `PRs in need of review`_ and follow these steps: .. _Contribution Guidelines: https://github.com/symfony/symfony/blob/master/CONTRIBUTING.md .. _Symfony's Release Schedule: http://symfony.com/doc/current/contributing/community/releases.html#schedule .. _Symfony Roadmap: https://symfony.com/roadmap +.. _Carson Bot: https://github.com/carsonbot/carsonbot From 0b5ce44c099eb665ad1e1a6ef72b988171b93a27 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Thu, 27 Aug 2015 18:07:49 +0200 Subject: [PATCH 8/8] Added instructions about testing a PR --- contributing/community/reviews.rst | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/contributing/community/reviews.rst b/contributing/community/reviews.rst index 309113910d1..c2d41b5b782 100644 --- a/contributing/community/reviews.rst +++ b/contributing/community/reviews.rst @@ -154,8 +154,16 @@ Pick a pull request from the `PRs in need of review`_ and follow these steps: #. **Test the Code** Take your project from step 3 and test whether the PR works properly. + Replace the Symfony vendor by the code in the PR by running the following Git + commands. Insert the PR ID for the ```` placeholders: - TODO: precise steps + .. code-block:: text + + $ cd vendor/symfony/symfony + $ git fetch origin pull//head:pr + $ git checkout pr + + Now you can test the project against the code in the PR. #. **Update the PR Status**