feature: Introduce Laravel-specific rules #6441
Conversation
szepeviktor
changed the title
szepeviktor
changed the title
| public function getDefinition(): FixerDefinitionInterface | ||
| { | ||
| return new FixerDefinition( | ||
| 'Annotations must respect the following order: @param, @return, and @throws.', |
There was a problem hiding this comment.
I believe we should rather generalise phpdoc_order instead of making framework-specific rule. This will help to avoid having lot of logic duplicated or to allow user to use 2 almost identical rules.
I have a feeling that this could be extended for other proposed rules
There was a problem hiding this comment.
Thank you for your comment.
Please tell me what to do with what. I don't know what these 3 classes do.
It is a copy-paste job from Laravel Pint. Thank you!
There was a problem hiding this comment.
I see! PhpdocOrderFixer: param -> throws -> return
PhpdocLaravelOrderFixer: param -> return -> throws
What do you mean by "generalise"?
Develop a 3rd class that contains common parts of these two??
FYI I do not write code daily, so $speed === 1 line/day 🐌
There was a problem hiding this comment.
In general, when we have a wish to do almost the same, but with a slight different outcome, to match different users expectations, we have one rule (single class), that we mark configurable. You may grep for ConfigurableFixerInterface to see examples.
Yet, if you are not original author of those fixers and you took it from another repo, I would suggest to incorporate original creator of it, to not violate his coderights/license.
I guess (I didn't checked deeply) that you copied them from laravel/pint repo, and if it is the case, I would prefer to wait for outcome of laravel/pint#50 before we move forward with it.
There was a problem hiding this comment.
Suggestion:
- let's have smaller PRs that are focusing on single job only. I would suggest to simplify this PR to focus on one rule only, and then move to other rule when first one is merged
- let's introduce configuration option to the fixer rather than duplicating it
- we gonna need some new utests to cover new functionality
There was a problem hiding this comment.
It would be to nice if a developer would jump in - I mostly do the thinking.
There was a problem hiding this comment.
If you have someone from Laravel community to help, please let them know (i guess it will be them who would benefit from changes).
If not really, and you are not the one who can work on this PR, then I suggest to close it until someone eager to do it would arrive.
There was a problem hiding this comment.
|
I'm happy to have those features incorporated into main repo. Anyone, if you feel up to prepare PR with it, please go ahead! Meanwhile, closing as discussed in #6441 (comment) |
Please be patient with me. This is the 1st time I use a computer :)