Skip to content
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

Jump to bottom

[book] controller ch review, part 1 #6349

Merged
merged 2 commits into from
Mar 17, 2016
Merged

[book] controller ch review, part 1 #6349

merged 2 commits into from
Mar 17, 2016

Conversation

paxyknox
Copy link
Contributor

Q A
Doc fix? yes
New docs? no
Applies to all
Fixed tickets x
  • tried to better form the concept of controller methods and controller classes - nothing new was edded just better formed
  • changes in styling, wording, typos

@paxyknox
controller ch review, part 1
7a3255e 
changes in styling
tried to beter form the concept of controller methods and controller classes
wording, typos
@paxyknox
added missing ref in routing ch
888e47e
@weaverryan
Copy link
Member

WOW! I'll review all of your pr's this week!

@paxyknox
Copy link
Contributor Author

Thank you! I have also Doctrine in Template chapter in store but first this.
I hope the PR are small enough! :)

@weaverryan weaverryan merged commit 888e47e into symfony:2.3 Mar 17, 2016
weaverryan added a commit that referenced this pull request Mar 17, 2016
@weaverryan
minor #6349 [book] controller ch review, part 1 (Talita Kocjan Zager)
87df252 
This PR was merged into the 2.3 branch.

Discussion
----------

[book] controller ch review, part 1

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | all
| Fixed tickets | x

- tried to better form the concept of controller methods and controller classes - nothing new was edded just better formed
- changes in styling, wording, typos

Commits
-------

888e47e added missing ref in routing ch
7a3255e controller ch review, part 1
weaverryan
weaverryan reviewed Mar 17, 2016
View reviewed changes
book/controller.rst
@@ -143,7 +150,7 @@ Mapping a URL to a Controller

The new controller returns a simple HTML page. To actually view this page
in your browser, you need to create a route, which maps a specific URL path
to the controller:
to the controller::
Copy link
Member

Choose a reason for hiding this comment

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

It's only 2 :: right before a php block (and then we don't need the .. code-block:: php

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted!

weaverryan added a commit that referenced this pull request Mar 17, 2016
@weaverryan
[#6349][#6351][#6352]
6214eb6 
Making minor tweaks after reading through the changes in those PR's
@weaverryan
Copy link
Member

Hey @paxyknox!

Phew! Ok I just merged everything in and made some changes here: sha: 6214eb6. If you see anything weird, let me know or (better) make a new PR. I like most of your changes - and just made additional changes to some of them.

And yes, the PR's were pretty easy to review :). Here's what I can tell you:

  1. Just make 1 PR for 1 chapter (instead of multiple). Because these cover so much, but don't make HUGE changes, I merge them and then review the sections you changed. I merged all 3 PR's at once, and they conflicted just a little with each other. So, just make one :).

  2. Re-wording is always subjective. If you are sure that something should be re-worded, then do it! But if you're not sure if your new wording is a big improvement, skip it: that saves us time reviewing and merging some "sideways" changes (things that are maybe just "slightly" better... or maybe not better at all).

Thanks!

weaverryan added a commit that referenced this pull request Mar 17, 2016
@weaverryan
Merge branch '2.3' into 2.7
e7246b6 
* 2.3: (24 commits)
  [#6365] Removing extra :
  Added minor clarification
  [#6360] Minor changes
  [#6349][#6351][#6352]
  Editing the Doctrine section to improve readability.
  Minor corrections
  Fixed typo
  Fix escaping of backtick inside double back-quotes
  Removed server:stop code block for 2.3
  Removed the PR table example (this is now included by GitHub template)
  Updated link to Translatable Extension
  [reference] [constraints] added missing colon character for Image constraint documentation in YAML format.
  Editing the Doctrine section to improve readability.
  Removed info about reducing visibility for private
  Updated link to Translatable Extension
  Editing the Doctrine section to improve readability.
  typo
  controller ch review, part 3
  typo
  controller ch review, part 2
  ...
@paxyknox paxyknox deleted the book-controller branch March 18, 2016 10:26
@paxyknox
Copy link
Contributor Author

I reviewed the merge it looks perfect! :)

Thank you for this instructions on how to make better PR, I really appreciate it!

xabbuh added a commit that referenced this pull request Mar 26, 2016
@xabbuh
Merge branch '2.7' into 2.8
a064e17 
* 2.7: (32 commits)
  Fixed wrong code examples for Isbn constraint
  unused use instructions
  Fix typo in SwitchUserListener file name
  Changed folder name to lowercase (best practises)
  [#6365] Removing extra :
  Add a note about enabling DebugBundle to use VarDumper inside Symfony
  Update introduction.rst
  Added minor clarification
  Changed folder name to lowercase (best practises)
  Fixed typo in path
  [#6360] Minor changes
  [#6349][#6351][#6352]
  Editing the Doctrine section to improve readability.
  Minor corrections
  Fixed typo
  Fix escaping of backtick inside double back-quotes
  Removed server:stop code block for 2.3
  Removed the PR table example (this is now included by GitHub template)
  Updated link to Translatable Extension
  [reference] [constraints] added missing colon character for Image constraint documentation in YAML format.
  ...

Conflicts:
	book/controller.rst
xabbuh added a commit that referenced this pull request Mar 26, 2016
@xabbuh
Merge branch '2.8' into 3.0
d2b6569 
* 2.8: (37 commits)
  Fixed wrong code examples for Isbn constraint
  Calling the parent implementation is mandatory.
  unused use instructions
  Fix typo in SwitchUserListener file name
  Reworded the example about $deep param
  Changed folder name to lowercase (best practises)
  [#6365] Removing extra :
  Add a note about enabling DebugBundle to use VarDumper inside Symfony
  Update introduction.rst
  Added minor clarification
  Changed folder name to lowercase (best practises)
  Fixed typo in path
  [#6360] Minor changes
  [#6349][#6351][#6352]
  Update "bootstrap.php.cache" to "autoload.php"
  Editing the Doctrine section to improve readability.
  Minor corrections
  Fixed typo
  Fix escaping of backtick inside double back-quotes
  Made list of types more consistent
  ...

Conflicts:
	book/installation.rst
	book/testing.rst
xabbuh added a commit that referenced this pull request Mar 26, 2016
@xabbuh
Merge branch '3.0'
8deb817 
* 3.0: (38 commits)
  Fixed wrong code examples for Isbn constraint
  Calling the parent implementation is mandatory.
  unused use instructions
  Fix typo in SwitchUserListener file name
  Reworded the example about $deep param
  Changed folder name to lowercase (best practises)
  [#6365] Removing extra :
  Add a note about enabling DebugBundle to use VarDumper inside Symfony
  Update introduction.rst
  Added minor clarification
  Changed folder name to lowercase (best practises)
  Fixed typo in path
  [#6360] Minor changes
  [#6349][#6351][#6352]
  Update "bootstrap.php.cache" to "autoload.php"
  Editing the Doctrine section to improve readability.
  Minor corrections
  Fixed typo
  Fix escaping of backtick inside double back-quotes
  Made list of types more consistent
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@paxyknox @weaverryan