10000 Added page "Community Reviews" by webmozart · Pull Request #5480 · symfony/symfony-docs · GitHub
[go: up one dir, main page]
Skip to content

Added page "Community Reviews" #5480

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
Closed

Added page "Community Reviews" #5480

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b91191a
Added initial draft of "Community Reviews" page
webmozart Jul 1, 2015
65a5a94
Added link to PRs in need of review
webmozart Jul 1, 2015
899c782
Improved sample comment
webmozart Jul 1, 2015
d08ccf8
Fixed case
webmozart Jul 1, 2015
2d7622b
Fixed numbering
webmozart Jul 2, 2015
62a4e85
Wording
webmozart Jul 2, 2015
d13a03b
Integrated PR comments
webmozart Jul 20, 2015
0b5ce44
Added instructions about testing a PR
webmozart Aug 27, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Integrated PR comments
  • Loading branch information
@webmozart
webmozart committed Jul 20, 2015
commit d13a03b0e70b063bc8b270d2bd22ff9e89480503
2 changes: 2 additions & 0 deletions contributing/code/patches.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
~~~~~~~~~~~~~~~~~~~

Expand Down
63 changes: 35 additions & 28 deletions contributing/community/reviews.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand All @@ -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
-----------------------
Expand All @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we put focus on the Symfony SE fork first? Imo many bugs are easily reproducable by describing some simple steps or by giving some small example code snippet. The thing is, we should make it as easy as possible to contribute and forking the Standard Edition is obviously more work than writing text that provides the same details.

So I think we should keep this paragraph, but just resort a bit which approach we put focus on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah ... i think we should classify the bugs a bit more to suggest when an SE fork is the sensible way to provide a reproducible test case .. in the end for a simple bug a description will be more efficient than having to download and run an SE fork (though what would be awesome is if platform.sh would provide some infrastructure to quickly spin up such forks for core devs).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my experience there is almost no bug that is easier to reproduce without a SE fork than with. Even if a bug sounds simple, you need to set up the environment, test whether the bug can be reproduced, if not (often the case) understand why etc. I personally have switched to asking for SE forks even for simple bugs. This way the risk of wasting time is much lower.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I shared the same thought as @lsmith77 and @xabbuh, but I think that forks are really done too little, and we should experiment with trying to encourage them. Yes, it's more work for contributors, but it may also be more satisfying for them to ship some code with the bug in it, and ultimately, this is an effort to relieve burden on the core team (and a fork is the best way). Let's try it this way for now.


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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above about the SE fork

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: <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: <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
Expand Down Expand Up @@ -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 <contributing-code-pull-request>`.

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 <releases>` 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should explain here that you need to differ between BC breaking changes and BC compatible changes given that we have development branches for 2.x and 3.x, shouldn't we?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1


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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just note that we will be able to (partially) automate some of these aspects eventually ..


Expand All @@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[...] "Before"/"After" examples [...]

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we don't really need to say much more here anyways. We can probably just remove this TODO line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for an average user there's much more to say. Sure, for us it's obvious, but the goal is to lower the barrier to entrance so more guidance = better IMO


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: <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: <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.
Expand All @@ -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

Expand All @@ -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
0