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

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
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
Fixed numbering
  • Loading branch information
@webmozart
webmozart committed Jul 2, 2015
commit 2d7622be96cf93725cf84ebe0342f61ad5613e34
8 changes: 4 additions & 4 deletions contributing/community/reviews.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
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

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

Choose a reason for hiding this comment

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

upgrade instructions?

4. **Test the Code**
5. **Test the Code**

Take your project from step 2 and test whether the PR works properly.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess step 3 (ie. Reproduce the Problem)?

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

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: <status>`` in
Expand Down