-
Notifications
You must be signed in to change notification settings - Fork 65
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
Conversation
600396d to
d303c9b
Compare
There was a problem hiding this 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.
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 :/
There was a problem hiding this 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.
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.
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.
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.
08f9886 to
44d80c4
Compare
Rebased on master and merged fixup commits
4cf3ecf to
03387dd
Compare
c9f40ac to
a043477
Compare
Ready to review ✔️
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 :)
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.
Sure, good luck with those last test 👍 They're the worst :D
Hi, I decided on a bit different approach: #65
- Add the methods to the Node interface
- 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.
Hey, that's good news 👍
I'll try to update my PR in Rector
Uh oh!
There was an error while loading. Please reload this page.
Closes #11