Added More Phpdoc Fixers #887

GrahamCampbell · 2014-12-16T23:53:43Z

TODO:

I THINK WE'RE DONE! :)

keradus · 2014-12-17T00:05:40Z

Symfony/CS/DocBlock/DocBlock.php

+     *
+     * @return bool
+     */
+    private function isAnnotation($content)


this looks like static

ceeram · 2014-12-17T01:06:01Z

phpdoc_group_tags_fixer as name perhaps? i think its more clear then phpdoc
separate, as that could mean separate phpdocs from code around it as well,
also it does not really separate tags, but its more correctly grouping tags

2014-12-17 1:29 GMT+01:00 Coveralls notifications@github.com:

[image: Coverage Status] https://coveralls.io/builds/1622212

Coverage increased (+0.49%) when pulling 4ca6c6c
4ca6c6c
on GrahamForks:1.5-phpdoc-separation into 10a42d0
10a42d0
on FriendsOfPHP:1.5.

—
Reply to this email directly or view it on GitHub
#887 (comment)
.

GrahamCampbell · 2014-12-18T14:45:20Z

Flysystem regression testing: thephpleague/flysystem#351.

GrahamCampbell · 2014-12-18T16:22:16Z

Running on symfony revealed a few issues:

The order of the throws statements should not change, but the move of the returns annotation was correct.

EDIT: FIXED

The separation fixer should not totally remove docblocks. This was caused by the forward and backward passes we made to look for blank lines before the real docblock content. We need to be careful not to delete everything if there is no content.

Before:

    /**
     *
     */

After:

Also, note how the end result also has unwanted whitespace.

EDIT: FIXED

Issues with deleted content. This happens when we're looking for the next annotation, but the description from the previous annotation has been separated. We need to improve how we detect the annotation description has finished. A blank line of docblock is not sufficient.

EDIT: FIXED

Ok, this is odd...

EDIT: FIXED

keradus · 2014-12-19T20:05:50Z

Oh my... huge progress from last time I see this PR !

GrahamCampbell · 2014-12-19T20:11:11Z

:)

keradus · 2014-12-19T20:21:08Z

Could you explain (in code ;) ) why the used fixers order are needed? What do we gain by it ?

GrahamCampbell · 2014-12-19T20:22:37Z

I thought I had. Err, well. The trim fixer needs to run quite late because the return void fixer can leave behind blank docblock lines if someone has separated it from the param annotations for example.

keradus · 2014-12-19T20:22:52Z

Symfony/CS/DocBlock/Annotation.php

+ * with this source code in the file LICENSE.
+ */
+
+namespace Symfony\CS\DocBlock;


please, create a testsuit in phpunit.xml.dist for DocBlock namespace

I, personally, would set "general" first and then all others alphabetically.
But for me it doesn't really matters...

Done:

<testsuites> <testsuite name="general"> <directory>./Symfony/CS/Tests/</directory> <exclude> <directory>./Symfony/CS/Tests/Fixer/</directory> <directory>./Symfony/CS/Tests/Fixtures/</directory> <directory>./Symfony/CS/Tests/Tokenizer/</directory> </exclude> </testsuite> <testsuite name="fixer"> <directory>./Symfony/CS/Tests/Fixer/</directory> </testsuite> <testsuite name="tokenizer"> <directory>./Symfony/CS/Tests/Tokenizer/</directory> </testsuite> <testsuite name="docblock"> <directory>./Symfony/CS/Tests/DocBlock/</directory> </testsuite> </testsuites>

GrahamCampbell · 2014-12-19T20:23:05Z

There are loads of cases here - difficult to write them all down.

keradus · 2014-12-19T20:24:39Z

Somehow you decided about that and not other order. Just please, write it down. If not then somebody in future may don't understand it and remove it.

GrahamCampbell · 2014-12-19T20:27:19Z

Somehow you decided about that and not other order. Just please, write it down. If not then somebody in future may don't understand it and remove it.

I'll add that to the todo as the order might change again after more testing on different repos.

keradus · 2014-12-19T20:28:16Z

great !

GrahamCampbell · 2014-12-19T21:09:27Z

Second round of testing on symfony:

Note how these lines are missing a space after the *. That's causing us to process them incorrectly.

This also brings to my attention that the behaviour with that annotation is undefined anyway since I've got no tests for it. Need to sort this...

EDIT: FIXED

GrahamCampbell · 2014-12-19T21:11:46Z

Symfony seems to like to keep @see and @author separated. I need to change the way I deal with this. Probably need a tag comparator class or something.

EDIT: FIXED

GrahamCampbell · 2014-12-19T23:38:02Z

@keradus Alright - where I'm at now - This is essentially feature complete, and current code coverage is 100%. I've still got some more unit tests to write for the docblocks classes in isolation, but we should be nearing completion.

GrahamCampbell · 2014-12-20T12:40:14Z

All the tests are done now. Now we need to discuss fixer names.

The following are new fixers introduced by this pull:

phpdoc_no_return_void

@return void and @return null annotations should be omitted from phpdocs.

phpdoc_separation

Annotations in phpdocs should be grouped together so that annotations of the same type immediately follow each other, and annotations of a different type are separated by a single blank line.

phpdoc_trim

Phpdocs should start and end with content, excluding the very fist and last line of the docblocks.

phpdoc_var

@var and @type annotations should not contain the variable name.

phpdoc_order

Annotations in phpdocs should be ordered so that param annotations come first, then throws annotations, then return annotations.

keradus · 2014-12-20T12:52:27Z

phpdoc_no_return_void
so no void? in descr I see null too. maybe phpdoc_no_empty_return ?

Other names are good for me

keradus · 2014-12-20T12:53:02Z

phpdoc_var
what about fixer that standardize @var vs @type
?

GrahamCampbell · 2014-12-20T12:58:27Z

phpdoc_var currently removes the variable name from the annotation. If we were to add a fixer to change @type -> @var, then I guess they both need better names.

I've renamed that other fixer.

GrahamCampbell · 2014-12-20T13:22:16Z

Renaming phpdoc_var to phpdoc_var_without_name.

GrahamCampbell · 2014-12-20T13:50:12Z

Just added phpdoc_type_to_var and phpdoc_var_to_type.

keradus · 2014-12-20T14:23:59Z

Symfony/CS/Tests/Fixer/Contrib/PhpdocVarToTypeFixerTest.php

+
+    public function testSingleLine()
+    {
+        $expected = <<<'EOF'


why a nowdoc instead of simply '<?php ...' ?

GrahamCampbell · 2015-02-06T20:34:50Z

@keradus Is there any chance you could finish this off for me. I don't think I'll be able to do anything here in the next 24 hours. Sorry.

keradus · 2015-02-06T20:41:05Z

@GrahamCampbell you know that when I could help I do it, I send PRs into yours few times, event into this one.
The problem here is that you set some priorities that I don't understand. And without answering my questions about them I simply can't help, even when I want to.
The questions are collected in my comment.
When you will answer the questions about priority then I could help it in one or another way. Not before :(

GrahamCampbell · 2015-02-08T19:20:30Z

Just rebased.

keradus · 2015-02-10T20:20:40Z

ping @GrahamCampbell

GrahamCampbell · 2015-02-10T20:33:02Z

I'll rebase again, look into the failing tests, then address those remaining issues.

keradus · 2015-02-10T20:36:33Z

cool, any chances to see you on gitter ?

keradus · 2015-02-12T20:21:46Z

ping @GrahamCampbell

keradus · 2015-02-17T19:56:45Z

Please see https://github.com/GrahamForks/PHP-CS-Fixer/pull/6

GrahamCampbell · 2015-02-17T20:05:30Z

Awesome @keradus. :)

keradus · 2015-02-17T20:35:03Z

Please rebaser to get rid of that merge commit and squash your commits into one, there should be one commit per one author.

keradus · 2015-02-18T17:43:52Z

ping @GrahamCampbell

GrahamCampbell · 2015-02-18T17:44:52Z

@keradus Will do very shortly. :)

GrahamCampbell · 2015-02-18T17:49:56Z

@keradus Done!

keradus · 2015-02-18T18:03:35Z

Thank you @GrahamCampbell. Great work here! ;)

@keradus

This PR was merged into the 1.5 branch. Discussion ---------- Added More Phpdoc Fixers TODO: * [x] Added phpdoc_seperation * [x] Fix the behaviour with some annotations like the phpunit ones - introduced a tag class * [x] Refactor the docblock classes to model the situation more clearly * [x] Add tests for the `Tag` class * [x] Add tests for the `DocBlock` class * [x] Added phpdoc_order * [x] Perform regression testing on league/flysystem * [x] Based on results from testing, make improvements * [x] Add phpdoc_var fixer * [x] Perform regression testing on symfony/symfony * [x] Based on results from testing, make improvements, add more test cases * [x] Move the phpdoc_order fixer to contrib level * [x] Add a phpdoc_trim fixer at symfony level, partly taken from phpdoc_separation * [x] Added phpdoc_no_return_void * [x] Review fixer orders * [x] Perform regression testing on guzzle/guzzle * [x] `@see`, `@link`, `@deprecated`, `@since`, `@author`, `@copyright`, and `@license` annotations should be kept together rather than forced apart by the separation fixer * [x] Support annotations with descriptions including bank lines * [x] Perform regression testing on doctrine/common * [x] Perform regression testing on guzzle/guzzle and symfony/symfony again * [x] Fix more bugs, and add more test cases * [x] Perform regression testing on flysystem guzzle, doctrine, symfony again * [x] Analyse current code coverage and cleanup code * [x] Add tests for the `TagComparator` * [x] Add missing `DocBlock` class tests * [x] Add tests for the `Line` class * [x] Add tests for the `Annotation` class * [x] Test on dingo/api, factory-muffin, aws sdk v2 and v3 * [x] Discuss fixer names * [x] Add fixers to change var to type and type to var * [x] Add a fixer to ensure short descriptions end in a `.`, `?`, or `!` * [x] Review by @keradus * [x] Cleanup the docblock classes * [x] Add a fixer to remove package and subpackage annotations * [x] Complete reviews and testing I THINK WE'RE DONE! :) Commits ------- 0006059 Enhance tests of order of phpdoc fixers 9f3aab8 Added more phpdoc fixers

GrahamCampbell · 2015-02-18T18:04:16Z

Pleasure. :)

keradus · 2015-02-18T18:42:22Z

Merging for over 30 minutes but merged into 2.0 as well.
Almost 5.2k assertions on it ;)

GrahamCampbell · 2015-02-18T18:42:58Z

Wow. :)

GrahamCampbell · 2015-02-18T18:43:21Z

What's the timescale on the 1.5 release btw?

keradus · 2015-02-18T18:47:32Z

You know this was the last one PR into 1.5.
Next steps as always:

merge into next major to see if all new things are compatible [done]
prepare changelog [working on it]
release ;)

GrahamCampbell · 2015-02-18T18:54:10Z

Sweet!

keradus · 2015-02-18T18:59:07Z

Ow, and one more step, actually first step at all..
regression on Symfony 2.3 and ZF2.

But I have done it for 1.5 this morning merging everything locally ;) Even 2 PR were sent ;)

@fabpot

This PR was squashed before being merged into the 2.3 branch (closes #13066). Discussion ---------- [2.3] CS And DocBlock Fixes | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A ##### This pull request provides some cs and docblock fixes. @fabpot The second commit of the two applies the docblock fixes you wanted, using PHP-CS-Fixer/PHP-CS-Fixer#887. I made 3 or 4 minor manual changes because people had put bulleted lists in the "short description" which is not allowed, so we were ending up with colons followed by full stops. I've moved those lists to be part of a "long description". // cc @keradus Commits ------- 081dd99 [2.3] CS And DocBlock Fixes

keradus reviewed Dec 17, 2014
View reviewed changes

GrahamCampbell changed the title ~~Added PhpdocSeparationFixer~~ [WIP] Added PhpdocSeparationFixer Dec 17, 2014

keradus added kind/feature WIP labels Dec 17, 2014

GrahamCampbell changed the title ~~[WIP] Added PhpdocSeparationFixer~~ [WIP] Added Two More Phpdoc Fixers Dec 18, 2014

GrahamCampbell mentioned this pull request Dec 18, 2014

Docblock Fixes thephpleague/flysystem#351

Merged

GrahamCampbell changed the title ~~[WIP] Added Two More Phpdoc Fixers~~ [WIP] Added More Phpdoc Fixers Dec 19, 2014

keradus mentioned this pull request Dec 19, 2014

phpdoc_params alignment #147

Closed

keradus reviewed Dec 19, 2014
View reviewed changes

keradus reviewed Dec 20, 2014
View reviewed changes

GrahamCampbell and others added 2 commits February 18, 2015 17:46

Added more phpdoc fixers

9f3aab8

Enhance tests of order of phpdoc fixers

0006059

keradus removed the WIP label Feb 18, 2015

keradus merged commit 0006059 into PHP-CS-Fixer:1.5 Feb 18, 2015

GrahamCampbell deleted the 1.5-phpdoc-separation branch February 18, 2015 18:04

GrahamCampbell mentioned this pull request Feb 28, 2015

Readds some missing functionality thephpleague/booboo#13

Merged

GrahamCampbell mentioned this pull request Mar 25, 2015

[5.0] Removed Some Unused Imports laravel/framework#8134

Merged

Added More Phpdoc Fixers #887

Added More Phpdoc Fixers #887

Conversation

GrahamCampbell commented Dec 16, 2014

keradus Dec 17, 2014

Choose a reason for hiding this comment

GrahamCampbell Dec 17, 2014

Choose a reason for hiding this comment

ceeram commented Dec 17, 2014

GrahamCampbell commented Dec 18, 2014

GrahamCampbell commented Dec 18, 2014

keradus commented Dec 19, 2014

GrahamCampbell commented Dec 19, 2014

keradus commented Dec 19, 2014

GrahamCampbell commented Dec 19, 2014

keradus Dec 19, 2014

Choose a reason for hiding this comment

keradus Dec 19, 2014

Choose a reason for hiding this comment

GrahamCampbell Dec 19, 2014

Choose a reason for hiding this comment

GrahamCampbell commented Dec 19, 2014

keradus commented Dec 19, 2014

GrahamCampbell commented Dec 19, 2014

keradus commented Dec 19, 2014

GrahamCampbell commented Dec 19, 2014

GrahamCampbell commented Dec 19, 2014

GrahamCampbell commented Dec 19, 2014

GrahamCampbell commented Dec 20, 2014

keradus commented Dec 20, 2014

keradus commented Dec 20, 2014

GrahamCampbell commented Dec 20, 2014

GrahamCampbell commented Dec 20, 2014

GrahamCampbell commented Dec 20, 2014

keradus Dec 20, 2014

Choose a reason for hiding this comment

GrahamCampbell Dec 20, 2014

Choose a reason for hiding this comment

GrahamCampbell commented Feb 6, 2015

keradus commented Feb 6, 2015

GrahamCampbell commented Feb 8, 2015

keradus commented Feb 10, 2015

GrahamCampbell commented Feb 10, 2015

keradus commented Feb 10, 2015

keradus commented Feb 12, 2015

keradus commented Feb 17, 2015

GrahamCampbell commented Feb 17, 2015

keradus commented Feb 17, 2015

keradus commented Feb 18, 2015

GrahamCampbell commented Feb 18, 2015

GrahamCampbell commented Feb 18, 2015

keradus commented Feb 18, 2015

GrahamCampbell commented Feb 18, 2015

keradus commented Feb 18, 2015

GrahamCampbell commented Feb 18, 2015

GrahamCampbell commented Feb 18, 2015

keradus commented Feb 18, 2015

GrahamCampbell commented Feb 18, 2015

keradus commented Feb 18, 2015