Skip to content

Add attributes to each node#57

Closed
TomasVotruba wants to merge 4 commits intophpstan:masterfrom
TomasVotruba:tv-add-attributes
Closed

Add attributes to each node#57
TomasVotruba wants to merge 4 commits intophpstan:masterfrom
TomasVotruba:tv-add-attributes

Conversation

@TomasVotruba
Copy link
Copy Markdown
Contributor

@TomasVotruba TomasVotruba commented Feb 7, 2021 •
edited
Loading

Closes #11

@TomasVotruba TomasVotruba changed the title tv add attributes Add attributes to each node Feb 7, 2021
ondrejmirtes
ondrejmirtes requested changes Feb 7, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

I also expected the parser to fill in those attributes needed for a format-preserving printer :) Right now the nodes will come with zero attributes from the parser.

@TomasVotruba
Copy link
Copy Markdown
Contributor Author

TomasVotruba commented Feb 7, 2021

I'm trying to figure out how CI works here. It seems hidden in some layer that does not allow PHP 8 to run cs :/

TomasVotruba
TomasVotruba commented Feb 7, 2021
View reviewed changes
TomasVotruba
TomasVotruba commented Feb 7, 2021
View reviewed changes
JanTvrdik
JanTvrdik reviewed Feb 7, 2021
View reviewed changes
JanTvrdik
JanTvrdik reviewed Feb 7, 2021
View reviewed changes
@TomasVotruba TomasVotruba mentioned this pull request Feb 7, 2021
Closed
ondrejmirtes
ondrejmirtes requested changes Feb 8, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

Please modify the parser to actually fill in those attributes needed for a format-preserving printer, otherwise it's fine.

@TomasVotruba
Copy link
Copy Markdown
Contributor Author

TomasVotruba commented Mar 11, 2021

Hi Ondra,

life came into way and I have other priorities ATM, so I won't have time to play around printer. I don't have it working in my packages yet either.
But it would be very helpful to allow attributes in the nodes. Rector could drop ~40 classes that it uses to generate attribute-aware nodes and whole code would get clean-up. It would also ease testing for printer.

Would you be ok to merge this PR with attributes only and then later handle the printer?
If so, I will update the PR with your comments and make CI pass.

@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Mar 12, 2021

Can you please verify this PR is backwards compatible with your other code? Probably is because you'd just be overriding an existing method, but I wouldn't want to break your existing code by releasing this in 0.4.13.

@TomasVotruba
Copy link
Copy Markdown
Contributor Author

TomasVotruba commented Mar 12, 2021

I've tested it sample and it should be ok:
https://3v4l.org/D4PkT

If it will break, I'll checkout latest 0.9.x Rector release and relase a new version with compatible code.
I'll be here today so I can patch it promptly.

@TomasVotruba TomasVotruba force-pushed the tv-add-attributes branch 2 times, most recently from 08f9886 to 44d80c4 Compare March 12, 2021 12:32
@TomasVotruba
Copy link
Copy Markdown
Contributor Author

TomasVotruba commented Mar 12, 2021

Rebased on master and merged fixup commits

@TomasVotruba
Copy link
Copy Markdown
Contributor Author

TomasVotruba commented Mar 12, 2021

Ready to review ✔️

@TomasVotruba TomasVotruba mentioned this pull request Mar 13, 2021
Closed
@TomasVotruba
Copy link
Copy Markdown
Contributor Author

TomasVotruba commented Mar 14, 2021 •
edited
Loading

I've just ported this branch to Rector and it works well 👍 Ready to merge for me.

See rectorphp/rector#5841 (PHPStan is not relevant; it's failing, because now the classes are loaded twice to give this PR a priority)

This is especially good to see :)

image

@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Mar 14, 2021

Yeah, give me a bit of time, I'm doing big refactoring this weekend (https://twitter.com/OndrejMirtes/status/1370778018566832131) related to handling static type, I'll look into this once I'm finished.

@TomasVotruba TomasVotruba mentioned this pull request Mar 14, 2021
Merged
@TomasVotruba
Copy link
Copy Markdown
Contributor Author

TomasVotruba commented Mar 14, 2021

Sure, good luck with those last test 👍 They're the worst :D

@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Mar 18, 2021

Hi, I decided on a bit different approach: #65

  1. Add the methods to the Node interface
  2. Use a trait instead of class inheritance.

I'm still crediting you in the commit. Because of the first change, I'll have to release a new major version (0.5). I believe you'll be able to use the PR very easily anyway (rectorphp/rector#5841). Thanks.

@TomasVotruba TomasVotruba deleted the tv-add-attributes branch March 18, 2021 21:28
@TomasVotruba
Copy link
Copy Markdown
Contributor Author

TomasVotruba commented Mar 18, 2021

Hey, that's good news 👍

I'll try to update my PR in Rector

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

Reviewers

@JanTvrdik JanTvrdik JanTvrdik left review comments

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow attributes to make format preserving print piece of cake

3 participants

@TomasVotruba @ondrejmirtes @JanTvrdik