[Console] CS fix #53363
[Console] CS fix #53363
Conversation
OskarStark
commented
Jan 2, 2024
| Q | A |
|---|---|
| Branch? | 5.4 |
| Bug fix? | yes |
| New feature? | no |
| Deprecations? | no |
| Issues | -- |
| License | MIT |
| @@ -53,7 +53,7 @@ public static function fromString(string $inputStr, int $currentIndex): self | |||
| * Create an input based on an COMP_WORDS token list. | |||
| * | |||
| * @param string[] $tokens the set of split tokens (e.g. COMP_WORDS or argv) | |||
| * @param $currentIndex the index of the cursor (e.g. COMP_CWORD) | |||
| * @param int $currentIndex the index of the cursor (e.g. COMP_CWORD) | |||
There was a problem hiding this comment.
This is intentional. The type is optional and already given in de PHP code. We've done this in multiple locations (and I would even be in favor of removing all duplicate types from the PHPdoc)
There was a problem hiding this comment.
PHPStorm was complaining about it:
There was a problem hiding this comment.
I think we should be content, in 5.4 only two more needs to be fixed, which I've done in this PR.
I also created a PR for 6.3
If both are accepted and (up)merged, I will check 6.4, 7.0 and 7.1
There was a problem hiding this comment.
To me, that's a bug in PHPstorm.
There was a problem hiding this comment.
@wouterj Do you have a link to a discussion about this?
I consider such doc param to be incomplete and less readable: when reading a param's description, not having the type right before it requires to find and look at the corresponding parameter inside the signature to get the full picture
There was a problem hiding this comment.
Sure, it has always been part of our coding standard:
- Add PHPDoc blocks for classes, methods, and functions only when they add relevant information that does not duplicate the name, native type declaration or context (e.g. instanceof checks);
Discussed in the PR making the change: #41912 (review)
See also #46140 #46218 #46217 #44935 #45324 (a couple of these are rejected by you actually Robin 😉)
| @@ -92,9 +92,6 @@ public function prune() | |||
| return $pruned; | |||
| } | |||
|
|
|||
| /** | |||
| * {@inheritdoc} | |||
| */ | |||
There was a problem hiding this comment.
Those "inheritdoc" got removed by applying the fabbot patch, shall I revert?
There was a problem hiding this comment.
any value to keep it? i would suggest to remove all.
and if anything, I would suggest to switch to #[Override] (probably as alignment for whole project, not this single place only)
| @@ -129,8 +129,6 @@ public function advance() | |||
|
|
|||
| /** | |||
| * Finish the indicator with message. | |||
| * | |||
| * @param $message | |||
There was a problem hiding this comment.
I think that's the only change we might wanna keep. See @wouterj's comment.
Also 5.4 is for bug fixes only, and this is not one.
7643b0a to
e9483bc
Compare
|
Thank you @OskarStark. |