-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat: Configurable phpDoc tags for FQCN processing #7628
feat: Configurable phpDoc tags for FQCN processing #7628
Conversation
|
||
Allowed types: ``array`` | ||
|
||
Default value: ``['param', 'phpstan-param', 'phpstan-property', 'phpstan-property-read', 'phpstan-property-write', 'phpstan-return', 'phpstan-var', 'property', 'property-read', 'property-write', 'psalm-param', 'psalm-property', 'psalm-property-read', 'psalm-property-write', 'psalm-return', 'psalm-var', 'return', 'see', 'throws', 'var']`` |
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.
LGTM, I would personally remove @see
which is required to be FQCN in some coding standards and very often is not a FQCN alone, it is very often a method name, which is not a "type" which this fixer is about.
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.
Yes, but as a second condition fixer matches simple FQCNs with a regex, so URLs or methods won't be processed 🙂.
'phpstan-param', | ||
'phpstan-property', | ||
'phpstan-property-read', | ||
'phpstan-property-write', |
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.
what would be the case of phpstan-property-read
enabled, but phpstan-property-write
disabled?
imagine someone actually use the config, phpstan introducing new annotation - they need to manually add the annotation here.
I would rather have a group of annotations like @phpstan
or phpstan*
for all phpstan-based annotations
(same for others)
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.
There are more phpstan-*
and psalm-*
annotations, but these have different purpose. I only included these related to types, tool-specific variants of regular phpDoc ones. I doubt new annotations will emerge 🙂.
Maybe we can focus on regular phpDoc tags, and automatically add prefixed ones 🤔? Like when someone includes param
and automatically phpstan-param
and psalm-param
are included. It would be more logical to me than wildcards, because you may want to process param
but not var
(prefixed variants). The other alternative is to ignore tool-specific variants in default config and just let people configure it by themselves, but as far as I remember other fixers handle these too.
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.
like, tbh I see only 2 cases myself - want to process annotations (any of them) or not. boolean config - do we have request to go granular and support some, but not all annotations?
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.
For example @see
is something that I think can be processed, and there was a voice it shouldn't be. Also, having this configurable makes it possible to clear large codebases step by step (by enabling one tag after another and providing partial diffs). List of tags was previously hardcoded and there was a request to add new tags - having it configurable allows people to do it in a flexible way, without changing the fixer's code.
It may be better to provide a list of tags to disallow this for, and then a flag to enable or not fixing at all in phpdoc. There are use cases also for handling names inside phpdoc but outside of the context of a tag. |
I am not sure if blacklist is better than whitelist, since these are a lot of phpDoc tags (or userland annotations) that should not be processed. In terms of on/off switch I plan to introduce another config option that will handle this, so here let's focus on the scope of the phpDoc processing. |
Fixes #7620 by providing ability to choose which phpDoc tags should be processed (in that case
@see
can be omitted and than problem is solved). I chose the approach with configurable list of phpDoc tags because it's more flexible - not only it allows disabling phpDoc support completely, but also allows narrowing/widening default scope by providing custom list of tags.