Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] CS fix#53363
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Console] CS fix#53363
Uh oh!
There was an error while loading.Please reload this page.
Conversation
OskarStark commentedJan 2, 2024
| Q | A |
|---|---|
| Branch? | 5.4 |
| Bug fix? | yes |
| New feature? | no |
| Deprecations? | no |
| Issues | -- |
| License | MIT |
| * | ||
| * @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) | ||
| * @paramint $currentIndex the index of the cursor (e.g. COMP_CWORD) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
PHPStorm was complaining about 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
To me, that's a bug in PHPstorm.
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 😉)
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.
Fair enough :)
| /** | ||
| * {@inheritdoc} | ||
| */ |
OskarStarkJan 3, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Those "inheritdoc" got removed by applying the fabbot patch, shall I revert?
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.
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)
Uh oh!
There was an error while loading.Please reload this page.
| /** | ||
| * Finish the indicator with message. | ||
| * | ||
| * @param $message |
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.
Useless
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 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.
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.
Done
nicolas-grekas commentedJan 4, 2024
Thank you@OskarStark. |