Improve naming in documentation #9354

kunicmarko20 · 2018-02-26T22:33:37Z

As the title says, I am trying to improve the naming of variables and some functions in the documentation. This started as just removing the abbreviations but while I needed to go through all files I tried to improve naming of everything that made sense to me.

greg0ire · 2018-02-26T22:35:43Z

components/config/definition.rst

        file_get_contents(__DIR__.'/src/Matthias/config/config.yml')
    );
-    $config2 = Yaml::parse(
+    $configExtra = Yaml::parse(


What about $extraConfig?

greg0ire · 2018-02-26T22:36:26Z

components/dependency_injection/compilation.rst

@@ -493,14 +493,14 @@ for these resources and use them as metadata for the cache::
    $containerConfigCache = new ConfigCache($file, $isDebug);

    if (!$containerConfigCache->isFresh()) {
-        $containerBuilder = new ContainerBuilder();
+        $container = new ContainerBuilder();


Why that change?

Becuase almost whole docs use $container and even https://github.com/symfony/dependency-injection/blob/master/Extension/Extension.php#L80 it is named as $container, no need to mix it.

I disagree here. This is one of those cases where variable naming is extremely important. A container builder is not a container, so the variable name is wrong.

So then $container in Extension is wrong also?

Yep, it should be $containerBuilder as well

adielcristo · 2018-02-27T01:48:41Z

components/routing.rst

@@ -41,7 +41,7 @@ your autoloader to load the Routing component::

    $context = new RequestContext('/');


here would be $requestContext too

Ah, I started renaming context to requestContext but then I saw context is used everywhere, not sure if I should rename all or revert this one.

kunicmarko20 · 2018-02-27T07:42:11Z

introduction/symfony1.rst

@@ -287,10 +287,10 @@ do the following:
        // app/config/routing.php
        use Symfony\Component\Routing\RouteCollection;

-        $collection = new RouteCollection();
-        $collection->addCollection($loader->import("@AcmeHelloBundle/Resources/config/routing.php"));
+        $routes = new RouteCollection();


Some of the docs use $routes and some use $collection for RouteCollection, I thought that $routes sounded better but maybe $routeCollection is the best option? Any suggestion on this?

I like $routes more than $collection and $routeCollection

Although I like $routes more, in the examples below the calls to $route->addCollection() and $route->setHost() make more sense as $routeCollection->addCollection() and $routeCollection->setHost().

I disagree :) $routes->addCollection() is easy to understand to me (just add a collection of routes to the existing routes) whereas $routeCollection->addCollection() is complicated (I'm replacing the existing collection? I'm merging two collections or creating a new collection? Is the result a route collection or a collection of route collections? etc.)

kunicmarko20 · 2018-02-27T07:48:47Z

components/security/firewall.rst


-    $firewall = new Firewall($map, $dispatcher);
+    $firewall = new Firewall($map, $eventDispatcher);


$map => $firewallMap

kunicmarko20 · 2018-02-27T07:49:57Z

console/style.rst

@@ -67,8 +67,8 @@ title of the command::

        protected function execute(InputInterface $input, OutputInterface $output)
        {
-            $io = new SymfonyStyle($input, $output);
-            $io->title('Lorem Ipsum Dolor Sit Amet');
+            $inputOutput = new SymfonyStyle($input, $output);


Do not really like this one but it is better than $io, maybe $symfonyStyle if that makes sense?

I think it's better to keep $io because we use it everywhere in the docs, the code, the Symfony Demo app, etc.

javiereguiluz

In other circumstances we'd say "closing because this is a merging nightmare". But I'm a big fan of good variable names, so I'll make the tedious merging and suffer the consequences 😓

javiereguiluz · 2018-02-27T07:56:11Z

components/console/events.rst

@@ -14,10 +14,10 @@ the wheel, it uses the Symfony EventDispatcher component to do the work::
    use Symfony\Component\Console\Application;
    use Symfony\Component\EventDispatcher\EventDispatcher;

-    $dispatcher = new EventDispatcher();
+    $eventDispatcher = new EventDispatcher();


Not sure about this one because in Symfony there's only one thing that can be a dispatcher: an event dispatcher.

I wasn't sure about this one but then I saw there are some places that it was already named $eventDispatcher

javiereguiluz · 2018-02-27T07:56:54Z

best_practices/forms.rst

-            $em = $this->getDoctrine()->getManager();
-            $em->persist($post);
-            $em->flush();
+            $entityManager = $this->getDoctrine()->getManager();


Not sure about this one because $em is pretty well known in the Symfony community and the alternative ($entityManager) is super long.

This is one of the reasons I started this PR 😄 I think this should be renamed and we should not continue to suggest abbreviations. (Also I found places where it already was $entityManager)

@javiereguiluz I've personally never been a big fan of $em and I also think most people that are familiar with $em won't get lost by having $entityManager instead

javiereguiluz · 2018-02-27T07:58:05Z

components/expression_language/caching.rst

@@ -36,8 +36,8 @@ in the object using the constructor::
    use Symfony\Component\ExpressionLanguage\ExpressionLanguage;
    use Acme\ExpressionLanguage\ParserCache\MyDatabaseParserCache;

-    $cache = new MyDatabaseParserCache(...);
-    $language = new ExpressionLanguage($cache);
+    $databaseCacheParser = new MyDatabaseParserCache(...);


This name looks wrong: it's not "a parser of db caches" but "a cache of db parser", right?

hm, I think you are right when I look at it one more time, maybe $databaseParserCache?

javiereguiluz · 2018-02-27T08:00:04Z

components/expression_language.rst

@@ -66,11 +66,11 @@ The main class of the component is

    use Symfony\Component\ExpressionLanguage\ExpressionLanguage;

-    $language = new ExpressionLanguage();
+    $expressionLanguage = new ExpressionLanguage();


I don't like either the original $language nor the new $expressionLanguage. Neither of them tells me what is this. But I guess the new name is better than the old one.

At least we are calling an apple an apple here :)

javiereguiluz · 2018-02-27T08:01:10Z

components/http_foundation/session_configuration.rst

@@ -45,8 +45,8 @@ Example usage::
    use Symfony\Component\HttpFoundation\Session\Storage\NativeSessionStorage;
    use Symfony\Component\HttpFoundation\Session\Storage\Handler\NativeFileSessionHandler;

-    $storage = new NativeSessionStorage(array(), new NativeFileSessionHandler());
-    $session = new Session($storage);
+    $nativeSessionStorage = new NativeSessionStorage(array(), new NativeFileSessionHandler());


I would rename it to $sessionStorage because "native" or not is just an implementation detail that can change at any moment.

javiereguiluz · 2018-02-27T08:02:39Z

components/security/authorization.rst

@@ -121,10 +121,10 @@ on a "remember-me" cookie, or even authenticated anonymously?
    use Symfony\Component\Security\Core\Authentication\Token\AnonymousToken;
    use Symfony\Component\Security\Core\Authentication\Token\RememberMeToken;

-    $anonymousClass = AnonymousToken::class;
-    $rememberMeClass = RememberMeToken::class;
+    $anonymousToken = AnonymousToken::class;


These should be $anonymousTokenClass and $rememberMeTokenClass because they are classes, not tokens. Or better yet: why don't we remove these useless variables and moved their values to the class constructor?

I thought about that but the point of PR is better naming? I saw some places where I could just move the values but I did not know if it would be appropriate for this PR?

javiereguiluz · 2018-02-27T08:04:16Z

components/translation/custom_formats.rst

@@ -46,10 +46,10 @@ create the catalog that will be returned::
                }
            }

-            $catalogue = new MessageCatalogue($locale);
-            $catalogue->add($messages, $domain);
+            $messageCatalogue = new MessageCatalogue($locale);


Just wondering: if later we replace $catalogue by $translations ... maybe here we can replace $messageCatalogue by $translation? Maybe is not a good idea.

Not sure really, the other place was importing the translations I thought that it was appropriate to rename it like that and here we are working with $messageCatalogue, I can change it but I am not sure?

javiereguiluz · 2018-02-27T08:04:56Z

console/command_in_controller.rst

@@ -50,11 +50,11 @@ Run this command from inside your controller via::
            ));

            // You can use NullOutput() if you don't need the output
-            $output = new BufferedOutput();
-            $application->run($input, $output);
+            $bufferedOutput = new BufferedOutput();


I disagree with this renaming. The fact that the output is buffered is just an implementation detail that can change at any time. The important thing is that this is a console output, so $output looks OK to me.

Agree, will be reverted.

kunicmarko20 · 2018-02-27T08:20:39Z

Is there any chance that I can help with merging when the time comes? I wouldn't mind doing it and I really want to see this one get merged.

javiereguiluz · 2018-02-27T08:33:00Z

@kunicmarko20 sorry but that can't happen because only the Doc Team members have permission to merge.

adielcristo · 2018-02-27T12:58:28Z

console/style.rst

@@ -67,8 +67,8 @@ title of the command::

        protected function execute(InputInterface $input, OutputInterface $output)
        {
-            $io = new SymfonyStyle($input, $output);
-            $io->title('Lorem Ipsum Dolor Sit Amet');
+            $inputOutput = new SymfonyStyle($input, $output);


$ioHelper would be more descriptive

I like it, but Javier said:

I think it's better to keep $io because we use it everywhere in the docs, the code, the Symfony Demo app, etc

tbh I did not find $io anywhere else, I will check again tonight. @javiereguiluz what do you think?

Frankly it's one of the rare cases where I prefer the short name because it's such a common name even outside of Symfony. $inputOutput is very verbose and a bit confusing with $intput, $output next to it. I could settle with $ioHelper though

kunicmarko20 · 2018-02-27T18:53:48Z

Some of the names have been updated according to comments, some are still waiting for a reply.

theofidry · 2018-02-27T19:04:29Z

bundles/configuration.rst

-        $def = $container->getDefinition('acme.social.twitter_client');
-        $def->replaceArgument(0, $config['twitter']['client_id']);
-        $def->replaceArgument(1, $config['twitter']['client_secret']);
+        $twitterClient = $container->getDefinition('acme.social.twitter_client');


$twitterClientDefinition. I remember getting confused the first time I came across definitions because of a similar naming as it was named as if it was the service but was not the service

I would rather revert it to $definition I saw some places where that name is used.

true, unless there is multiple ones you might not care about a better naming

theofidry

Big 👍 it's a nice work! I think those kind of naming is very important for new comers especially and quite a few terms there were source of confusion

theofidry · 2018-02-27T19:05:04Z

components/config/definition.rst

@@ -534,7 +534,7 @@ tree with ``append()``::

    public function addParametersNode()
    {
-        $builder = new TreeBuilder();
+        $treeBuilder = new TreeBuilder();
        $node = $builder->root('parameters');


missing renaming here as well

nice catch.

theofidry · 2018-02-27T19:05:47Z

components/dependency_injection/compilation.rst

@@ -493,14 +493,14 @@ for these resources and use them as metadata for the cache::
    $containerConfigCache = new ConfigCache($file, $isDebug);

    if (!$containerConfigCache->isFresh()) {
-        $containerBuilder = new ContainerBuilder();
+        $container = new ContainerBuilder();


Yep, it should be $containerBuilder as well

theofidry · 2018-02-27T19:06:42Z

components/expression_language.rst

@@ -66,11 +66,11 @@ The main class of the component is

    use Symfony\Component\ExpressionLanguage\ExpressionLanguage;

-    $language = new ExpressionLanguage();
+    $expressionLanguage = new ExpressionLanguage();


At least we are calling an apple an apple here :)

theofidry · 2018-02-27T19:08:27Z

components/finder.rst

@@ -176,12 +176,12 @@ Sort the result by name or by type (directories first, then files)::

 You can also define your own sorting algorithm with ``sort()`` method::

-    $sort = function (\SplFileInfo $a, \SplFileInfo $b)
+    $sortFunction = function (\SplFileInfo $a, \SplFileInfo $b)


not a big fan of this one. $sort describes that it is, I don't think you need the type in the name. When you have $x = ['name 1', 'name 2'] an appropriate name for $x would be $names not $stringNames unless special circumstances where the type marks the difference

I agree, what about maybe moving it directly into ->sort() instead of assigning it to a variable?

I would personally do it directly yeah unless there is more of such functions and ends up making it hard to read. Otherwise I think the naming ($sort) is perfectly correct. I'll leave @javiereguiluz make the call there :)

theofidry · 2018-02-27T19:10:23Z

create_framework/dependency_injection.rst

-    $sc = new DependencyInjection\ContainerBuilder();
-    $sc->register('context', Routing\RequestContext::class);
-    $sc->register('matcher', Routing\Matcher\UrlMatcher::class)
+    $container = new DependencyInjection\ContainerBuilder();


$containerBuilder. I would review the usage of $container elsewhere as ContainerBuilder is very different from $container

I agree but then we come back here https://github.com/symfony/dependency-injection/blob/master/Extension/Extension.php#L80 where the type is ContainerBuilder and name is container

IMO that should be changed and it's very confusing :/

I'm a little confused. I thought that ContainerBuilder (besides the misleading name) would be a Container too, as it extends from Container.

kunicmarko20 · 2018-02-27T21:11:05Z

I renamed all $container to $containerBuilder where we have a new instance of ContainerBuilder the only place I left it as $container is in the examples that show the real code e.g. the Dependency Injection Extension

javiereguiluz · 2018-02-28T16:47:20Z

@kunicmarko20 please tell me when you think this is ready so I can better prepare for merging this. Thanks!

kunicmarko20 · 2018-02-28T17:14:47Z

@javiereguiluz if there is nothing else that needs to be changed on my side, it is ready.

javiereguiluz · 2018-03-01T14:57:06Z

Let's merge this. Wish me luck!

This PR was merged into the 2.7 branch. Discussion ---------- Improve naming in documentation As the title says, I am trying to improve the naming of variables and some functions in the documentation. This started as just removing the abbreviations but while I needed to go through all files I tried to improve naming of everything that made sense to me. Commits ------- 219b7f8 improve naming

javiereguiluz · 2018-03-01T15:47:40Z

@kunicmarko20 the merge has been completed up to master branch. the idea would be to now do the same for 2.8 branch, then I merge up. Then for 3.4 branch and merge up, 4.0 and merge up and finally, master. But I understand if you are tired :)

By the way, when you get to 3.4 please double check form/data_transformers.rst because there were some changes related to ObjectManager that I don't know if I merged them correctly.

kunicmarko20 · 2018-03-01T15:52:16Z

Thank you for doing this. So I would need to do the same check for those 3 branches but create PR after the merge of previous? 2.8 -> 3.4 -> 4.0?

javiereguiluz · 2018-03-01T15:55:34Z

That's correct. If you can, the next step would be to fix 2.8 variable names (in reality, you are only fixing the wrongly variable names that didn't exist in 2.7) and then I'll merge things up again. Later, we repeat for 3.4, 4.0 and master 😓

kunicmarko20 · 2018-03-01T15:58:35Z

Ok, sorry for making you do this but it is worth it! 😄

This PR was merged into the 2.8 branch. Discussion ---------- Improve naming in documentation Continuing the #9354, this time on branch 2.8, there weren't a lot of things that needed a change I guess the initial PR got almost everything 😄 Commits ------- 87d9046 improve naming

This PR was merged into the 3.4 branch. Discussion ---------- Improve naming in documentation Continuing #9354, this time on branch 3.4. Commits ------- 51ddf41 improve naming

carsonbot added the Status: Needs Review label Feb 26, 2018

greg0ire reviewed Feb 26, 2018

View reviewed changes

adielcristo reviewed Feb 27, 2018

View reviewed changes

kunicmarko20 commented Feb 27, 2018

View reviewed changes

javiereguiluz approved these changes Feb 27, 2018

View reviewed changes

adielcristo reviewed Feb 27, 2018

View reviewed changes

kunicmarko20 force-pushed the improve_naming branch 2 times, most recently from 7d1d5db to 14bb7b3 Compare February 27, 2018 18:52

theofidry reviewed Feb 27, 2018

View reviewed changes

kunicmarko20 force-pushed the improve_naming branch 2 times, most recently from 32ad701 to a6d8ad1 Compare February 27, 2018 21:08

improve naming

219b7f8

kunicmarko20 force-pushed the improve_naming branch from a6d8ad1 to 219b7f8 Compare February 27, 2018 21:33

javiereguiluz added Status: Reviewed and removed Status: Needs Review labels Mar 1, 2018

javiereguiluz added this to the 2.7 milestone Mar 1, 2018

javiereguiluz merged commit 219b7f8 into symfony:2.7 Mar 1, 2018

kunicmarko20 deleted the improve_naming branch March 1, 2018 14:58

kunicmarko20 mentioned this pull request Mar 11, 2018

Improve naming in documentation #9423

Merged

kunicmarko20 mentioned this pull request Mar 12, 2018

Improve naming in documentation #9430

Merged

kunicmarko20 mentioned this pull request Aug 13, 2018

SonataAdmin without User Bundle docs sonata-project/SonataAdminBundle#5187

Merged

2 tasks

		@@ -41,7 +41,7 @@ your autoloader to load the Routing component::

		$context = new RequestContext('/');


		$firewall = new Firewall($map, $dispatcher);
		$firewall = new Firewall($map, $eventDispatcher);

Uh oh!

Improve naming in documentation #9354

Improve naming in documentation #9354

Uh oh!

Conversation

kunicmarko20 commented Feb 26, 2018

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!

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!

javiereguiluz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kunicmarko20 Feb 27, 2018 • 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!

kunicmarko20 Feb 27, 2018 • 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!

kunicmarko20 Feb 27, 2018 • 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!

javiereguiluz Feb 27, 2018 • 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!

kunicmarko20 Feb 27, 2018 •

edited

Loading

kunicmarko20 Feb 27, 2018 •

edited

Loading

kunicmarko20 Feb 27, 2018 •

edited

Loading

javiereguiluz Feb 27, 2018 •

edited

Loading