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

DocBlock - handle removing of first or last line #4035

Open
wants to merge 7 commits into
base: 2.12
from

Conversation

Projects
None yet
3 participants
@kubawerlos
Copy link
Contributor

kubawerlos commented Oct 13, 2018

For a PHPDoc:

/** @foo
 * @bar
 */

removing annotation foo with Annotation::remove will also remove /** which will result with broken PHPDoc syntax, leading to PHP syntax error after fixing.

What should happen in such case (and similar with closing */)? Should we aim ro changing DocBlock constructor to split into pieces differently?

@dmvdbrugge

This comment has been minimized.

Copy link
Contributor

dmvdbrugge commented Oct 15, 2018

I think the remove methods should take begin/end of block into account when performing their action, I feel like I've worked around this before, maybe even in an open PR (not sure).

It will not hinder removing an entire (empty) docblock anyway because that should just remove the Token.

@SpacePossum

This comment has been minimized.

Copy link
Member

SpacePossum commented Oct 15, 2018

I would be expect;

/**
 * @bar
 */

as result.
And;

/** @bar*/

for

/** @foo @bar */

maybe we can add this test as well;

/** @bar
 * @foo */
@dmvdbrugge

This comment has been minimized.

Copy link
Contributor

dmvdbrugge commented Oct 15, 2018

I don't think /** @foo @bar */ valid syntax to begin with though. @bar is and should be considered part of the text of @foo.

@SpacePossum

This comment has been minimized.

Copy link
Member

SpacePossum commented Oct 15, 2018

alright, please lets have that case in the tests as well (even if untouched by the rule)

@kubawerlos

This comment has been minimized.

Copy link
Contributor Author

kubawerlos commented Oct 15, 2018

I think the remove methods should take begin/end of block into account when performing their action

@dmvdbrugge neither Assertion not Line is aware if it contains the beginning of the PHPDoc (multiple /** inside of PHPDoc are valid syntax) so I went one level higher to DocBlock class itself.

After removing some lines (no matter if via Assertion or directly) the getContent function must return valid content.

@kubawerlos kubawerlos changed the title [RFC] Annotation class - removing annotation may break PHPDoc syntax DocBlock - handle removing of first or last line Oct 15, 2018

@dmvdbrugge

This comment has been minimized.

Copy link
Contributor

dmvdbrugge commented Oct 16, 2018

neither Assertion not Line is aware if it contains the beginning of the PHPDoc

Sorry but you're wrong: line is aware. In fact, I remembered I already had a PR for this: #3893
It's been sitting there, quietly waiting review...

@kubawerlos

This comment has been minimized.

Copy link
Contributor Author

kubawerlos commented Oct 16, 2018

The Line::isTheStart may return true while not being start line e.g.

<?php
/**
/**
 * Foo
 */
@dmvdbrugge

This comment has been minimized.

Copy link
Contributor

dmvdbrugge commented Oct 18, 2018

That however (if true) is a bug in Line::isTheStart that should be fixed. Also, I don't think that's a valid docblock. The GH-syntax-highlighting even breaks on it.

@kubawerlos kubawerlos force-pushed the kubawerlos:bugfix/docblock-removing-annotation branch from 906345a to e339efd Nov 12, 2018

@kubawerlos

This comment has been minimized.

Copy link
Contributor Author

kubawerlos commented Nov 12, 2018

It is a valid DocBlock (from the PHP tokenizer perspective) and to fix it we would have to pass the information if it's the start in constructor - not sure if we would want to go this way.

SpacePossum and others added some commits Dec 27, 2018

@SpacePossum SpacePossum removed the WIP label Jan 2, 2019

kubawerlos added some commits Jan 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.