Skip to content
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

PhpdocAnnotationWithoutDotFixer - Handle empty line in comment #3882

Merged
merged 1 commit into from Aug 19, 2018
Merged

PhpdocAnnotationWithoutDotFixer - Handle empty line in comment #3882

merged 1 commit into from Aug 19, 2018

Conversation

kubawerlos
Copy link
Contributor

Solves #3857 (comment) - yet, feels too easy - I'm pessimistic about this one - can we easily trick it? @SpacePossum @dmvdbrugge what do you think?

Copy link
Contributor

@dmvdbrugge dmvdbrugge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Learning from all the comments I've gotten before, I don't think this is the way to go, it indeed feels a bit hacky.

Also, @SpacePossum said in #3857 he considers this case a priority-issue, for which I made #3879 which should fix not only PhpdocAnnotationWithoutDotFixer but all Phpdoc*Fixers

'<?php
/**
* This is a broken phpdoc
* @param string string Surprisingly, it is a string.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can see that the fixer works by the removal of the dot, but this should be string $str instead of string string, and then Surprisingly will be lowercased as well.

@SpacePossum SpacePossum self-assigned this Jul 9, 2018
@SpacePossum
Copy link
Contributor

SpacePossum commented Jul 11, 2018

The solutions, this one and resolving the priority, don't have to be exclusive, sorry if I caused confusion about that.

For this PR, I like the direction 👍
How would something like

    /**
     * @return bool|null Returns `true` if the class has a single-column ID
                        and Returns `false` otherwise.
     */'

be fixed in the new situation?

@kubawerlos
Copy link
Contributor Author

@SpacePossum not nicely - PhpCsFixer\DocBlock\DocBlock::getAnnotations will lose 2nd line at all.

@SpacePossum
Copy link
Contributor

can you add this to the tests?

    /**
     * @return bool|null Returns `true` if the class has a single-column ID.
                         Returns `false` otherwise.
     */

If the fixer removes the dot after ID but not after otherwise I'm happy, if it doesn't removes any dot than it is fine as well for me. It is nice to have a test to see how the fixer handles invalid PHPDocs, that is all :)

@SpacePossum
Copy link
Contributor

Thanks for the test, this PR looks good to me.
Maybe another member can have look as well?

@SpacePossum SpacePossum added this to the 2.12.3 milestone Jul 30, 2018
@keradus
Copy link
Member

keradus commented Aug 10, 2018

@kubawerlos , can you rebase please ? we have some conflicts due to recent merging session

@keradus keradus added RTM Ready To Merge and removed RTM Ready To Merge labels Aug 16, 2018
@keradus
Copy link
Member

keradus commented Aug 19, 2018

Thank you @kubawerlos.

@keradus keradus merged commit 14190a4 into PHP-CS-Fixer:2.12 Aug 19, 2018
keradus added a commit that referenced this pull request Aug 19, 2018
…mment (kubawerlos)

This PR was merged into the 2.12 branch.

Discussion
----------

PhpdocAnnotationWithoutDotFixer - Handle empty line in comment

Solves #3857 (comment) - yet, feels too easy - I'm pessimistic about this one - can we easily trick it? @SpacePossum @dmvdbrugge what do you think?

Commits
-------

14190a4 PhpdocAnnotationWithoutDotFixer - Handle empty line in comment
@kubawerlos kubawerlos deleted the fix/phpdoc-annotation-without-dot-fixer-to-handle-empty-lines-in-comment branch August 20, 2018 05:06
keradus added a commit that referenced this pull request Oct 14, 2018
SpacePossum added a commit that referenced this pull request Oct 18, 2018
This PR was squashed before being merged into the 2.12 branch (closes #4027).

Discussion
----------

PhpdocAnnotationWithoutDotFixer - add failing cases

introduced in #3882

ref symfony/symfony#28814
ref symfony/symfony#28817

Commits
-------

330fe39 PhpdocAnnotationWithoutDotFixer - add failing cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants