Added More Phpdoc Fixers #887

Merged
merged 2 commits into from Feb 18, 2015

Conversation

Projects
None yet
5 participants
@GrahamCampbell
Contributor

GrahamCampbell commented Dec 16, 2014

TODO:

  • Added phpdoc_seperation
  • Fix the behaviour with some annotations like the phpunit ones - introduced a tag class
  • Refactor the docblock classes to model the situation more clearly
  • Add tests for the Tag class
  • Add tests for the DocBlock class
  • Added phpdoc_order
  • Perform regression testing on league/flysystem
  • Based on results from testing, make improvements
  • Add phpdoc_var fixer
  • Perform regression testing on symfony/symfony
  • Based on results from testing, make improvements, add more test cases
  • Move the phpdoc_order fixer to contrib level
  • Add a phpdoc_trim fixer at symfony level, partly taken from phpdoc_separation
  • Added phpdoc_no_return_void
  • Review fixer orders
  • Perform regression testing on guzzle/guzzle
  • @see, @link, @deprecated, @since, @author, @copyright, and @license annotations should be kept together rather than forced apart by the separation fixer
  • Support annotations with descriptions including bank lines
  • Perform regression testing on doctrine/common
  • Perform regression testing on guzzle/guzzle and symfony/symfony again
  • Fix more bugs, and add more test cases
  • Perform regression testing on flysystem guzzle, doctrine, symfony again
  • Analyse current code coverage and cleanup code
  • Add tests for the TagComparator
  • Add missing DocBlock class tests
  • Add tests for the Line class
  • Add tests for the Annotation class
  • Test on dingo/api, factory-muffin, aws sdk v2 and v3
  • Discuss fixer names
  • Add fixers to change var to type and type to var
  • Add a fixer to ensure short descriptions end in a ., ?, or !
  • Review by @keradus
  • Cleanup the docblock classes
  • Add a fixer to remove package and subpackage annotations
  • Complete reviews and testing

I THINK WE'RE DONE! :)

@keradus

View changes

Symfony/CS/DocBlock/DocBlock.php
+ *
+ * @return bool
+ */
+ private function isAnnotation($content)

This comment has been minimized.

@keradus

keradus Dec 17, 2014

Member

this looks like static

@keradus

keradus Dec 17, 2014

Member

this looks like static

This comment has been minimized.

@GrahamCampbell

GrahamCampbell Dec 17, 2014

Contributor

👍

@ceeram

This comment has been minimized.

Show comment
Hide comment
@ceeram

ceeram Dec 17, 2014

Contributor

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)
.

Contributor

ceeram commented Dec 17, 2014

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 GrahamCampbell changed the title from Added PhpdocSeparationFixer to [WIP] Added PhpdocSeparationFixer Dec 17, 2014

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

@GrahamCampbell GrahamCampbell referenced this pull request in thephpleague/flysystem Dec 18, 2014

Merged

Docblock Fixes #351

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Dec 18, 2014

Contributor

Flysystem regression testing: thephpleague/flysystem#351.

Contributor

GrahamCampbell commented Dec 18, 2014

Flysystem regression testing: thephpleague/flysystem#351.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Dec 18, 2014

Contributor

Running on symfony revealed a few issues:


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

EDIT: FIXED


  1. 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


  1. 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.
    image

EDIT: FIXED


  1. Ok, this is odd...
    image

EDIT: FIXED

Contributor

GrahamCampbell commented Dec 18, 2014

Running on symfony revealed a few issues:


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

EDIT: FIXED


  1. 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


  1. 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.
    image

EDIT: FIXED


  1. Ok, this is odd...
    image

EDIT: FIXED

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

@keradus keradus referenced this pull request Dec 19, 2014

Closed

phpdoc_params alignment #147

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Dec 19, 2014

Member

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

Member

keradus commented Dec 19, 2014

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

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Dec 19, 2014

Contributor

:)

Contributor

GrahamCampbell commented Dec 19, 2014

:)

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Dec 19, 2014

Member

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

Member

keradus commented Dec 19, 2014

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

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Dec 19, 2014

Contributor

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.

Contributor

GrahamCampbell commented Dec 19, 2014

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.

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

This comment has been minimized.

@keradus

keradus Dec 19, 2014

Member

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

@keradus

keradus Dec 19, 2014

Member

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

This comment has been minimized.

@keradus

keradus Dec 19, 2014

Member

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

@keradus

keradus Dec 19, 2014

Member

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

This comment has been minimized.

@GrahamCampbell

GrahamCampbell Dec 19, 2014

Contributor

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

GrahamCampbell Dec 19, 2014

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Dec 19, 2014

Contributor

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

Contributor

GrahamCampbell commented Dec 19, 2014

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

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Dec 19, 2014

Member

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.

Member

keradus commented Dec 19, 2014

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

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Dec 19, 2014

Contributor

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.

Contributor

GrahamCampbell commented Dec 19, 2014

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

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Dec 19, 2014

Member

great !

Member

keradus commented Dec 19, 2014

great !

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Dec 19, 2014

Contributor

Second round of testing on symfony:

image

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

Contributor

GrahamCampbell commented Dec 19, 2014

Second round of testing on symfony:

image

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

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Dec 19, 2014

Contributor

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.

image

EDIT: FIXED

Contributor

GrahamCampbell commented Dec 19, 2014

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.

image

EDIT: FIXED

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Dec 19, 2014

Contributor

@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.

Contributor

GrahamCampbell commented Dec 19, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Dec 20, 2014

Contributor

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.
Contributor

GrahamCampbell commented Dec 20, 2014

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

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Dec 20, 2014

Member

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

Other names are good for me

Member

keradus commented Dec 20, 2014

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

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Dec 20, 2014

Member

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

Member

keradus commented Dec 20, 2014

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

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Dec 20, 2014

Contributor

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.

Contributor

GrahamCampbell commented Dec 20, 2014

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

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Dec 20, 2014

Contributor

Renaming phpdoc_var to phpdoc_var_without_name.

Contributor

GrahamCampbell commented Dec 20, 2014

Renaming phpdoc_var to phpdoc_var_without_name.

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Dec 20, 2014

Contributor

Just added phpdoc_type_to_var and phpdoc_var_to_type.

Contributor

GrahamCampbell commented Dec 20, 2014

Just added phpdoc_type_to_var and phpdoc_var_to_type.

@keradus

View changes

Symfony/CS/Tests/Fixer/Contrib/PhpdocVarToTypeFixerTest.php
+
+ public function testSingleLine()
+ {
+ $expected = <<<'EOF'

This comment has been minimized.

@keradus

keradus Dec 20, 2014

Member

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

@keradus

keradus Dec 20, 2014

Member

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

This comment has been minimized.

@GrahamCampbell

GrahamCampbell Dec 20, 2014

Contributor

fixed

@GrahamCampbell

GrahamCampbell Dec 20, 2014

Contributor

fixed

@keradus keradus added this to the v1.5 milestone Dec 20, 2014

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Dec 21, 2014

Contributor

Just updated the todo list with another fixer. :)

Contributor

GrahamCampbell commented Dec 21, 2014

Just updated the todo list with another fixer. :)

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Dec 21, 2014

Contributor

Done.

Contributor

GrahamCampbell commented Dec 21, 2014

Done.

@keradus

View changes

Symfony/CS/Fixer/Symfony/PhpdocShortDescriptionFixer.php
+
+ $char = substr($content, - 1);
+
+ return '.' === $char || '!' === $char || '?' === $char;

This comment has been minimized.

@keradus

keradus Dec 21, 2014

Member

in_array ?

@keradus

keradus Dec 21, 2014

Member

in_array ?

@keradus

View changes

Symfony/CS/Fixer/Symfony/PhpdocShortDescriptionFixer.php
+ $content = rtrim($line->getContent());
+ if (!$this->isCorrectlyFormatted($content)) {
+ $line->setContent($content.".\n");
+ $tokens[$index]->setContent($doc->getContent());

This comment has been minimized.

@keradus

keradus Dec 21, 2014

Member

isn't it a $token ?

@keradus

keradus Dec 21, 2014

Member

isn't it a $token ?

@keradus

View changes

Symfony/CS/Fixer/Contrib/PhpdocOrderFixer.php
+ $content = $this->moveParamAnnotations($content);
+ $content = $this->moveReturnAnnotations($content);
+ // persist the content at the end
+ $tokens[$index]->setContent($content);

This comment has been minimized.

@keradus

keradus Dec 21, 2014

Member

isn't it a $token ?

@keradus

keradus Dec 21, 2014

Member

isn't it a $token ?

@keradus

View changes

Symfony/CS/Fixer/Contrib/PhpdocVarToTypeFixer.php
+ $line->setContent(str_replace('@var', '@type', $line->getContent()));
+ }
+
+ $tokens[$index]->setContent($doc->getContent());

This comment has been minimized.

@keradus

keradus Dec 21, 2014

Member

isn't it a $token ?

@keradus

keradus Dec 21, 2014

Member

isn't it a $token ?

@keradus

View changes

Symfony/CS/Fixer/Symfony/PhpdocNoEmptyReturnFixer.php
+ $this->fixAnnotation($doc, $annotation);
+ }
+
+ $tokens[$index]->setContent($doc->getContent());

This comment has been minimized.

@keradus

keradus Dec 21, 2014

Member

isn't it a $token ?

@keradus

keradus Dec 21, 2014

Member

isn't it a $token ?

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Jan 25, 2015

Member

@keradus We've got a failing test here. I assume that's because ¡ and ¿ need the multibyte string functions. How should I proceed?

I'll try to handle it tomorrow

Member

keradus commented Jan 25, 2015

@keradus We've got a failing test here. I assume that's because ¡ and ¿ need the multibyte string functions. How should I proceed?

I'll try to handle it tomorrow

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Jan 25, 2015

Member

@GrahamCampbell, maybe sth like

    private function isCorrectlyFormatted($content)
    {
        if (false !== strpos(strtolower($content), '{@inheritdoc}')) {
            return true;
        }

        return $content !== rtrim($content, '.!?¡¿');
    }

?

Member

keradus commented Jan 25, 2015

@GrahamCampbell, maybe sth like

    private function isCorrectlyFormatted($content)
    {
        if (false !== strpos(strtolower($content), '{@inheritdoc}')) {
            return true;
        }

        return $content !== rtrim($content, '.!?¡¿');
    }

?

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Jan 25, 2015

Contributor

@keradus I don't think it will. The rtrim function doesn't support multibyte strings. :(

Contributor

GrahamCampbell commented Jan 25, 2015

@keradus I don't think it will. The rtrim function doesn't support multibyte strings. :(

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Jan 25, 2015

Member

Not so sure about that mb_trim you refefred. Especially things like '\\\\\\0'...

Yes, rtrim doesn't support multibyte string, it split ¡ into two bytes and then trim that two bytes at the end. It does the trick. Try it yourself ;)

Member

keradus commented Jan 25, 2015

Not so sure about that mb_trim you refefred. Especially things like '\\\\\\0'...

Yes, rtrim doesn't support multibyte string, it split ¡ into two bytes and then trim that two bytes at the end. It does the trick. Try it yourself ;)

@GrahamCampbell GrahamCampbell referenced this pull request in jeremeamia/super_closure Jan 26, 2015

Merged

Added the ability to sign and verify closure serializations. #43

+ */
+ public function getPriority()
+ {
+ // must be run before the PhpdocSeperationFixer

This comment has been minimized.

@keradus

keradus Jan 29, 2015

Member

Sorry but it is not obvious for me why, could you explain ?

@keradus

keradus Jan 29, 2015

Member

Sorry but it is not obvious for me why, could you explain ?

This comment has been minimized.

@GrahamCampbell

GrahamCampbell Jan 29, 2015

Contributor

The order fixer must be run first otherwise we're separating the annotations while they're in the wrong order, then ordering them which might break the separation.

@GrahamCampbell

GrahamCampbell Jan 29, 2015

Contributor

The order fixer must be run first otherwise we're separating the annotations while they're in the wrong order, then ordering them which might break the separation.

This comment has been minimized.

@keradus

keradus Jan 29, 2015

Member

Yeah, that makes sense. Thanks!

@keradus

keradus Jan 29, 2015

Member

Yeah, that makes sense. Thanks!

This comment has been minimized.

@GrahamCampbell

GrahamCampbell Feb 10, 2015

Contributor

Will document this too...

@GrahamCampbell

GrahamCampbell Feb 10, 2015

Contributor

Will document this too...

@keradus

View changes

Symfony/CS/Fixer/Symfony/PhpdocParamsFixer.php
{
- return 'All items of the @param, @throws, @return, @var, and @type phpdoc tags must be aligned vertically.';
+ // should be run after all other phpdoc fixers
+ return -10;

This comment has been minimized.

@keradus

keradus Jan 29, 2015

Member

Sorry but it is not obvious for me why, could you explain ?
Again eg type vs var...

@keradus

keradus Jan 29, 2015

Member

Sorry but it is not obvious for me why, could you explain ?
Again eg type vs var...

This comment has been minimized.

@keradus

keradus Jan 29, 2015

Member

not tested at all

@keradus

keradus Jan 29, 2015

Member

not tested at all

@keradus

This comment has been minimized.

Show comment
Hide comment
Member

keradus commented Feb 3, 2015

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Feb 6, 2015

Member

anything @GrahamCampbell ? this is the last thing before release 1.5 and it would be really cool to see you here !

Member

keradus commented Feb 6, 2015

anything @GrahamCampbell ? this is the last thing before release 1.5 and it would be really cool to see you here !

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Feb 6, 2015

Contributor

@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.

Contributor

GrahamCampbell commented Feb 6, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Feb 6, 2015

Member

@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 :(

Member

keradus commented Feb 6, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Feb 8, 2015

Contributor

Just rebased.

Contributor

GrahamCampbell commented Feb 8, 2015

Just rebased.

@keradus

This comment has been minimized.

Show comment
Hide comment
Member

keradus commented Feb 10, 2015

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Feb 10, 2015

Contributor

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

Contributor

GrahamCampbell commented Feb 10, 2015

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

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Feb 10, 2015

Member

cool, any chances to see you on gitter ?

Member

keradus commented Feb 10, 2015

cool, any chances to see you on gitter ?

@keradus

This comment has been minimized.

Show comment
Hide comment
Member

keradus commented Feb 12, 2015

@keradus

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Feb 17, 2015

Contributor

Awesome @keradus. :)

Contributor

GrahamCampbell commented Feb 17, 2015

Awesome @keradus. :)

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Feb 17, 2015

Member

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

Member

keradus commented Feb 17, 2015

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

@keradus

This comment has been minimized.

Show comment
Hide comment
Member

keradus commented Feb 18, 2015

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Feb 18, 2015

Contributor

@keradus Will do very shortly. :)

Contributor

GrahamCampbell commented Feb 18, 2015

@keradus Will do very shortly. :)

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
Contributor

GrahamCampbell commented Feb 18, 2015

@keradus Done!

@keradus keradus removed the WIP label Feb 18, 2015

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Feb 18, 2015

Member

Thank you @GrahamCampbell. Great work here! ;)

Member

keradus commented Feb 18, 2015

Thank you @GrahamCampbell. Great work here! ;)

@keradus keradus merged commit 0006059 into FriendsOfPHP:1.5 Feb 18, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

keradus added a commit that referenced this pull request Feb 18, 2015

feature #887 Added More Phpdoc Fixers (GrahamCampbell, 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 GrahamCampbell deleted the 1.5-phpdoc-separation branch Feb 18, 2015

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Feb 18, 2015

Contributor

Pleasure. :)

Contributor

GrahamCampbell commented Feb 18, 2015

Pleasure. :)

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Feb 18, 2015

Member

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

Member

keradus commented Feb 18, 2015

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

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Feb 18, 2015

Contributor

Wow. :)

Contributor

GrahamCampbell commented Feb 18, 2015

Wow. :)

@GrahamCampbell

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Feb 18, 2015

Contributor

What's the timescale on the 1.5 release btw?

Contributor

GrahamCampbell commented Feb 18, 2015

What's the timescale on the 1.5 release btw?

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Feb 18, 2015

Member

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 ;)
Member

keradus commented Feb 18, 2015

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

This comment has been minimized.

Show comment
Hide comment
@GrahamCampbell

GrahamCampbell Feb 18, 2015

Contributor

Sweet!

Contributor

GrahamCampbell commented Feb 18, 2015

Sweet!

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Feb 18, 2015

Member

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 ;)

Member

keradus commented Feb 18, 2015

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 ;)

@GrahamCampbell GrahamCampbell referenced this pull request in thephpleague/booboo Feb 28, 2015

Merged

Readds some missing functionality #13

@GrahamCampbell GrahamCampbell referenced this pull request in laravel/framework Mar 25, 2015

Merged

[5.0] Removed Some Unused Imports #8134

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