Skip to content

Add attributes to each node #57

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

Closed
wants to merge 4 commits into from

Conversation

TomasVotruba
Copy link
Contributor

@TomasVotruba TomasVotruba commented Feb 7, 2021

Closes #11

@TomasVotruba TomasVotruba changed the title tv add attributes Add attributes to each node Feb 7, 2021
Copy link
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
Contributor Author

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 :/

Copy link
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
Contributor Author

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
Member

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

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

Rebased on master and merged fixup commits

@TomasVotruba
Copy link
Contributor Author

Ready to review ✔️

@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented Mar 14, 2021

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
Member

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
Copy link
Contributor Author

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

@ondrejmirtes
Copy link
Member

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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow attributes to make format preserving print piece of cake
3 participants