Skip to content

[TypeDeclaration] Do not remove multiple docblocks with comment on TypedPropertyFromAssignsRector#3263

Merged
samsonasik merged 9 commits intomainfrom
do-not-remove-multiple-comments
Jan 3, 2023
Merged

[TypeDeclaration] Do not remove multiple docblocks with comment on TypedPropertyFromAssignsRector#3263
samsonasik merged 9 commits intomainfrom
do-not-remove-multiple-comments

Conversation

@samsonasik
Copy link
Copy Markdown
Member

@samsonasik samsonasik commented Jan 1, 2023

Fixes rectorphp/rector#7693

Multiiple docblock should only remove last doc as detected one is the last:

    // A comment

    /**
     * Another comment
     */

-    /**
-     * @var \DateTime
-     */

For only comment + docblock, it remove the docblock part:

   // A comment
-    /**
-     * @var \DateTime
-     */

@samsonasik samsonasik marked this pull request as draft January 1, 2023 17:18
@samsonasik samsonasik force-pushed the do-not-remove-multiple-comments branch from 1876303 to d42ce3c Compare January 1, 2023 17:47
@samsonasik
Copy link
Copy Markdown
Member Author

Fixed 🎉 /cc @neveldo

@samsonasik samsonasik marked this pull request as ready for review January 1, 2023 17:47
@samsonasik
Copy link
Copy Markdown
Member Author

All checks have passed 🎉 @TomasVotruba it is ready for review.

@samsonasik samsonasik changed the title [TypeDeclaration] Do not remove multiple comments on TypedPropertyFromAssignsRector [TypeDeclaration] Do not remove multiple docblocks with comment on TypedPropertyFromAssignsRector Jan 1, 2023
@samsonasik samsonasik marked this pull request as draft January 2, 2023 10:20
@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Jan 2, 2023

The safest way seems mark previous docs as comment.

@samsonasik
Copy link
Copy Markdown
Member Author

I updated to mark Doc as Comment when not in last, as the used one is last 06b3f6e

@samsonasik samsonasik force-pushed the do-not-remove-multiple-comments branch from e6d60c3 to 45ab2dc Compare January 2, 2023 10:58
@samsonasik
Copy link
Copy Markdown
Member Author

Next todo: preserve spacing new line between docblocks/comments when calling PhpDocInfoFactory->createFromNode().

@TomasVotruba
Copy link
Copy Markdown
Member

I see. Is this ready for review?

@samsonasik
Copy link
Copy Markdown
Member Author

Yes, but not mergeable yet, as currently just calling:

PhpDocInfoFactory->createFromNode()

will remove the spacing, that will cause many unnecessary space removal for something like:.

/**
 * @license abc
 */

/**
 * @Annotation
 */

I can split into smaller PR for change this part only (comment + single docblock):

   // A comment
-    /**
-     * @var \DateTime
-     */

for non-multiple docblocks, wdyt?

@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Jan 2, 2023

I added failing fixture to preserve multiple docs when docs never changed 04929fb , it currently still removing space new line even docs never changed, which invalid.

 final class PreserveMultipleDocsNoChange
 {
     // A comment
-
     /**
      * Another comment
      */
-
     /**
      * @var class-string
      */
-    private $property;
+    private string $property;

@samsonasik samsonasik force-pushed the do-not-remove-multiple-comments branch from 4955e14 to e250ddd Compare January 3, 2023 10:19
…mAssignsRector

more fixture rules-tests

Fix

Fixed 🎉

final touch: eol

Final touch: return null on multiple docs

updated to mark Doc as Comment when not in last, as the used one is last

Final touch: clean up

[ci-review] Rector Rectify

apply setDocComment after set comments attribute

failing fixture preserve multiple docs no change

type

create another attribute to save previous docs
@samsonasik samsonasik force-pushed the do-not-remove-multiple-comments branch from 171bc08 to c6484ff Compare January 3, 2023 11:31
@samsonasik samsonasik force-pushed the do-not-remove-multiple-comments branch from 12ade3f to 548f144 Compare January 3, 2023 11:40
@samsonasik samsonasik marked this pull request as ready for review January 3, 2023 11:43
@samsonasik
Copy link
Copy Markdown
Member Author

samsonasik commented Jan 3, 2023

@TomasVotruba it finally resolved now 🎉 🎉 🎉 , on after phpdocinfo changed, when no longer children, it needs to apply

  • mark previous comments + docs as comments
  • renew last doc as new main doc

@samsonasik
Copy link
Copy Markdown
Member Author

All checks have passed 🎉 @TomasVotruba it is ready for review.

@samsonasik samsonasik merged commit 8c439cf into main Jan 3, 2023
@samsonasik samsonasik deleted the do-not-remove-multiple-comments branch January 3, 2023 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TypedPropertyFromAssignsRector seems to delete too many comments

3 participants