spotted minor EN mistakes and coherence issues between the different … #14496

Ca-Jou · 2020-10-28T15:34:09Z

…chapters

Nyholm

Thank you.

Im usually a suspect for grammar mistakes but I think none of these were mine =)

Nyholm · 2020-10-30T12:47:24Z

create_framework/http_kernel_httpkernelinterface.rst

@@ -64,6 +64,7 @@ PHP; it implements ``HttpKernelInterface`` and wraps another
        new HttpKernel\HttpCache\Store(__DIR__.'/../cache')
    );

+    /* $response variable used before and after this chapter -> keep it in this chapter for more global coherence? */


Are you suggesting to rewrite like this:

$response = $framework->handle($request); $response->send();

Yes, this is the way it is implemented in the chapters before and after this one, so might be nice to keep it this way here to improve global coherence of the code.

I agree with that.

Thank you

Nyholm · 2020-10-30T12:48:06Z

create_framework/http_kernel_httpkernelinterface.rst

@@ -46,8 +46,8 @@ Update your framework so that it implements this interface::
        }
    }

-Even if this change looks not too complex, it brings us a lot! Let's talk about one of
-the most impressive one: transparent :doc:`HTTP caching </http_cache>` support.
+Even if this change doesn't look like much, it brings us a lot! Let's talk about one of


Can we rephrase this differently?

Not to say that any change is "small" or "simple".

Maybe "Even if this change looks minor..." then?
It maintains the general idea that a little goes a long way, but doesn't suggest it's simple.
Or "With this change, a little goes a long way !"

Or "With this change, a little goes a long way !"

Perfect!

Context for other reviewers: The change is to add an interface.

Then I would go with:
"With this change, a little goes a long way! Let's talk about one of the most impressive upsides:"

Sounds. good. Thank you.

OskarStark · 2020-10-30T13:02:34Z

create_framework/event_dispatcher.rst

+    	/* all properties are now declared as private, they were protected until this chapter -> either there is a good reason and it might be useful to explain this change, or there could be better coherence by keeping them protected *or* private in the entire book */
+


Nice catch, I added a PR #14501 are there any other occurrences I didn't catch?

Excellent. These lines can now be removed.

Well done @OskarStark and @Ca-Jou 👍

@OskarStark not that I'm aware of.
@Nyholm sorry for the newb question, but it's my first PR ever: am I supposed to remove these lines in my PR (by committing once more or editing this commit) now that another one has been opened?

sorry for the newb question, but it's my first PR ever:

Congratulations. And what a great PR to start with =)

You can just make a new commit and push to the same branch (patch-create-framework). There is no need to be fancy and editing the commits etc.

wouterj

Thank you! This part of the docs does not get the attention it deserves. Great to have such detailed and correct fixes across the chapters!

create_framework/http_kernel_httpkernelinterface.rst

Nyholm · 2020-10-30T13:37:20Z

Let me know if I can help you with something. Im available in the Symfony slack

@Nyholm

…skarStark) This PR was merged into the 4.4 branch. Discussion ---------- Enhancement: Private member variables in code example Based on #14496 cc @Nyholm @Ca-Jou Commits ------- 8d1614e Enhancement: Private member variables in code example

javiereguiluz · 2020-10-30T14:23:49Z

@Ca-Jou thanks a lot for these super nice improvements! Thanks to reviewers too. What an amazing team work you all did in this PR! 👏

carsonbot added the Status: Needs Review label Oct 28, 2020

Nyholm reviewed Oct 30, 2020

View reviewed changes

OskarStark reviewed Oct 30, 2020

View reviewed changes

OskarStark mentioned this pull request Oct 30, 2020

Enhancement: Private member variables in code example #14501

Merged

wouterj reviewed Oct 30, 2020

View reviewed changes

create_framework/http_kernel_httpkernelinterface.rst Outdated Show resolved Hide resolved

javiereguiluz added Status: Reviewed and removed Status: Needs Review labels Oct 30, 2020

javiereguiluz added this to the 4.4 milestone Oct 30, 2020

javiereguiluz changed the base branch from 5.x to 4.4 October 30, 2020 14:23

spotted minor EN mistakes and coherence issues between the different …

ae45622

javiereguiluz force-pushed the patch-create-framework branch from 8416432 to ae45622 Compare October 30, 2020 14:23

javiereguiluz merged commit 0f7fd39 into symfony:4.4 Oct 30, 2020

		/* all properties are now declared as private, they were protected until this chapter -> either there is a good reason and it might be useful to explain this change, or there could be better coherence by keeping them protected or private in the entire book */

Uh oh!

spotted minor EN mistakes and coherence issues between the different … #14496

spotted minor EN mistakes and coherence issues between the different … #14496

Uh oh!

Conversation

Ca-Jou commented Oct 28, 2020

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ca-Jou Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wouterj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Nyholm commented Oct 30, 2020

Uh oh!

javiereguiluz commented Oct 30, 2020

Uh oh!

Uh oh!

Ca-Jou Oct 30, 2020 •

edited

Loading