-
Notifications
You must be signed in to change notification settings - Fork 2k
[Rector] Apply Rector run on utils/ directory #6225
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
[Rector] Apply Rector run on utils/ directory #6225
Conversation
| { | ||
| /** | ||
| * @var string | ||
| */ |
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.
@samsonasik Why does Rector remove this PHPDoc?
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.
It is @MGatner request to create custom rule to remove var from constant https://github.com/codeigniter4/CodeIgniter4/blob/develop/utils/Rector/RemoveVarTagFromClassConstantRector.php
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.
A constant is a constant 🤷♂️
I do remember that conversation but I'm also fine if we want to revisit this. I think it is unnecessary to type constants but people do it.
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.
ref #4766
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.
It is a common practice to put @var for constants.
I have never seen removing @var for constants.
But at least, PhpStorm does not need @var for constants.
So it does not seem to be inconvenient if it is removed.
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.
A constant is no longer a constant.
The underlying values may be mutable but it is still a fixed type at runtime. If for some awful reason somebody used your example I would expect a docblock explanation, not a type. Providing @var typing for static analysis is unnecessary because constants are always strongly-typed to whatever they are.
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.
Yeah, the type is fixed.
But the value is dynamic determined. Very interesting.
https://3v4l.org/dWAaW
I don't know how and when to use it, though.
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.
Object constants are strange to me. I can't even find the RFC that allowed them for a good example of why someone would want one 🤷♂️ To me the power of a constant is it's fixed, simple value.
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.
Here it is: https://wiki.php.net/rfc/new_in_initializers
Looks like it wasn't deliberately for constants, and is also limited strictly to global constants. The fact that the value for the class constant ends up being dynamic is somewhat of a syntax hack.
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.
Typed class constants is under discussion for 8.2: https://wiki.php.net/rfc/typed_class_constants
Apply Rector run on
utils/directory, not only/utils/Rectorso other files (PHPStan rules, check_tabs_in_rst.php), will be checked as well.Checklist: