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.

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

[cs] remove unused doc blocks #24931

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@TomasVotruba
Copy link
Contributor

TomasVotruba commented Nov 12, 2017

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

Follow-up to #24930

@@ -198,7 +198,7 @@ public function __construct(A $a, $foo = 'default_val', Lille $lille)
}
}
/*
/**
* Classes used for testing createResourceForClass
*/
class ClassForResource

This comment has been minimized.

@TomasVotruba

TomasVotruba Nov 12, 2017

Contributor

These docs were incorrectly interpretted by PHP CS Fixer as single line, thus fixing them to Doc Blocks

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Nov 14, 2017

Even if I'd love to remove all PHPdocs from all files ... I find it strange to keep some @param and remove others. This is confusing and it will make people submit PRs to add the missing @params in PHPdocs.

@TomasVotruba

This comment has been minimized.

Copy link
Contributor

TomasVotruba commented Nov 14, 2017

Some have added value, some don't and only duplicate information.
What do you suggest then?

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Nov 14, 2017

What you did is correct, but let me explain the problem I see (using as an example the first change you did):

Original

 /**
  * Adds the stack logger for a connection.
  *
  * @param string     $name
  * @param DebugStack $logger
  */
  public function addLogger($name, DebugStack $logger)

Proposed solution

 /**
  * Adds the stack logger for a connection.
  *
  * @param string $name
  */
  public function addLogger($name, DebugStack $logger)

Best solution

public function addLogger(string $name, DebugStack $logger)

We can't make the best solution to avoid BC issues, but the problem I see with the proposed solution is that people will think we made a mistake and forgot to add the $logger argument in the PHPdocs, because the method has 2 arguments but PHPdoc has only 1.

@stof

This comment has been minimized.

Copy link
Member

stof commented Nov 14, 2017

Yeah, I think partial phpdoc is confusing for users. We should keep an all-or-nothing approach for the parameter list IMO

@TomasVotruba

This comment has been minimized.

Copy link
Contributor

TomasVotruba commented Nov 14, 2017

I see what you mean. Is there any reason to have it such way apart historical reasons? I realize there were not such tools in the past, but today thanks to coding standard tools this is now easy to configure and improve.

It will make code much easier to read for users and easier modify in the future. Moving to Best solution as next step.

Merged Use Case

Similar PRs were merged to php-ai/php-ml lately. Removing over 1700 useless lines.

See especially these cases: https://github.com/php-ai/php-ml/pull/145/files#diff-1359851beb8da185b9d9adae86ab2ff7

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 27, 2017

people will think we made a mistake and forgot to add the $logger argument in the PHPdocs

This will not happen if fabbot generates the patch, because then, it will become an enforced policy.
So IMHO, we should do this change, but only when fabbot is able to generate the patch.

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Nov 27, 2017

* @param ObjectManager $manager
* @param mixed $queryBuilder
* @param string $class
* @param mixed $queryBuilder

This comment has been minimized.

@fabpot

fabpot Dec 11, 2017

Member

mixed params could be removed as well as mixed does not give any useful information.

This comment has been minimized.

@TomasVotruba

TomasVotruba Dec 19, 2017

Contributor

I'd rather keep this, since mixed type is comming to PHP and it would make easier transition to scalar type, as in string, int etc. before

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Dec 11, 2017

@TomasVotruba Can you also change the base to 2.7? Thanks.

Are you doing this by hand? Or with a tool? If you are using a tool/regexes/..., can you share it so that merging up to master will be easier?

@TomasVotruba

This comment has been minimized.

Copy link
Contributor

TomasVotruba commented Dec 19, 2017

@fabpot Here is all you asked for: https://www.tomasvotruba.cz/blog/2017/12/17/new-in-symplify-3-doc-block-cleaner-fixer/

Shall I run this rule on 2.7 with current setup then?

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 19, 2018

@TomasVotruba Can you share you setup? Such PRs are complex to merge as merging it to newest branches is a mess. So, if you can share you setup, I will be able to run it on all branches, limiting the time it takes to merge the change up. Thanks.

@TomasVotruba

This comment has been minimized.

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 19, 2018

@TomasVotruba I've just tried by it fixes only 15 files.

@TomasVotruba

This comment has been minimized.

Copy link
Contributor

TomasVotruba commented Jan 19, 2018

@fabpot I'm not aware of any major change that could cause this.
Could you share full install script and run command?

And version? Last is 3.1
On which branch?

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 19, 2018

I've tried on master and v3.2.2, which looks like the last one (from a git clone https://github.com/Symplify/EasyCodingStandard)

@TomasVotruba

This comment has been minimized.

Copy link
Contributor

TomasVotruba commented Jan 19, 2018

@fabpot I meant 3.2, yes.
I'll check it then

@TomasVotruba

This comment has been minimized.

Copy link
Contributor

TomasVotruba commented Jan 19, 2018

I tried it like this:

git clone github.com/symfony/symfony
cd ../symplify # monorepo, with 3.2.3 version (I fixed one variadics bug and just released patch version)
packages/EasyCodingStandard/bin/ecs check ../symfony/src/Symfony --clear-cache --config cleaning-symfony.neon

Where cleaning-symfony.neon

# cleaning-symfony.neon
checkers:
    - Symplify\CodingStandard\Fixer\Commenting\RemoveUselessDocBlockFixer

With this result:

result-1
files:

Then with --fix option I'll really get 200 changed files:

image

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 20, 2018

I think I forgot to pass the config file. Now it looks much better, but I get this error:

PHP Notice:  Undefined offset: 0 in /Users/fabien/Code/github/Symplify/EasyCodingStandard/vendor/symplify/better-reflection-docblock/src/Tag/TolerantParam.php on line 132

Notice: Undefined offset: 0 in /Users/fabien/Code/github/Symplify/EasyCodingStandard/vendor/symplify/better-reflection-docblock/src/Tag/TolerantParam.php on line 132

In FixerFileProcessor.php line 125:

  Fixing of "src/Symfony/Component/Translation/Dumper/FileDumper.php" file by "Symplify\CodingStandard\Fixer\Commenting\RemoveUselessDocBlockFixer" failed: substr() expects parameter 1 to be string,
  null given in file /Users/fabien/Code/github/Symplify/EasyCodingStandard/vendor/symplify/better-reflection-docblock/src/Tag/TolerantParam.php on line 132

Note that I got the config from your blog post.

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 20, 2018

Also, I noticed that it removes the blank line between @params and @throws/@return, how can I disable this behavior?

@TomasVotruba

This comment has been minimized.

Copy link
Contributor

TomasVotruba commented Jan 20, 2018

I think I forgot to pass the config file. Now it looks much better, but I get this error:

That's what I fixed. Do you have version 3.2.3?

Note that I got the config from your blog post.

Use the one provided here, just to be sure the main fixer works for you. The other 2 are just space fixers.

Also, I noticed that it removes the blank line between @params and @throws/@return, how can I disable this behavior?

I thinks so. Will create issue for that. It uses ReflectionDocBlock which renders all docblock in one format.

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 20, 2018

Ok, I forgot to run composer up as the fix was in a dep apparently :)

@fabpot fabpot referenced this pull request Jan 20, 2018

Closed

Remove uneeded docblocks #25859

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 20, 2018

See #25859 for the current state.

@fabpot fabpot closed this Jan 20, 2018

@TomasVotruba TomasVotruba deleted the TomasVotruba:cs-remove-unused-docs branch Jan 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment