do not resolve types if it's not possible #254
Conversation
voku
changed the title
|
@jaapio I see Exceptions like the following, since I added |
There was a problem hiding this comment.
@voku thanks a lot for this PR.
A lot has been changed. So it is kind of hard to see if we are not breaking anything, which could result in incoming issues. This code is kind of highly sensitive because it is used in so many projects. I want to be careful with large PR's.
Besides that, I'm not sure if any other tag rather then @param should have support for variadic and reference. I never used variadic properties? Unless I'm incorrect and you can show me some examples of variadic properties I would like to stick with the original behavior for the other tags.
If you discovered a bug in the original patch, a fix is more than welcome.
voku
force-pushed
the
fix_for_phpstorm_stubs
branch
from
1608708
to
acf538a
Compare
|
@jaapio 👍 you are right, I only saw the code that looks ~ the same in all this classes, but I reverted it. And here is only the fix for my current problem. |
|
Could you please add a test-case which reproduces the issue you had? |
|
@jaapio I added some more tests ... ... but in the tests I also saw a new issue where |
|
Thanks, when I have a quick look at the changes this looks good to me. I'm going to do another review soon. But I think we can merge this as is 👍 |
…stubs * upstream/master: Bump mockery Fix FQSEN resolving on see,covers,uses Improve test coverage
-> fix code style + psalm reported errors
-> fix code style v2
|
@jaapio By adding more unit tests I saw more strange test results. :-/ I don't know if I should revert everything back to the fix for my current problem, without the tests and extra fixes? What do you think? |
-> update the code coverage level
|
Thanks for adding all these test. With just quick look, it seems you are fixing some nice output issues here. for any future contributions I would like you to ask to split the PR into multiple smaller PR's. Which makes the review easier, but also allows us to merge things separate. So we can move faster :-) Thanks a lot for all the effort you are spending on this. I will try to review your changes as soon as possible. Unless something is wrong I will handle some changes that I would write in a different way. Could also be written as: Which makes it a bit more readable IMO. |
|
isn't that the same as |
The code mostly looks like this: if ($this->description) {
$description = $this->description->render();
} else {
$description = '';
}
$variableName = '';
if ($this->variableName) {
$variableName .= ($this->isReference ? '&' : '') . ($this->isVariadic ? '...' : '');
$variableName .= '$' . $this->variableName;
}
$type = (string) $this->type;
return $type
. ($variableName !== '' ? ($type !== '' ? ' ' : '') . $variableName : '')
. ($description !== '' ? ($type !== '' || $variableName !== '' ? ' ' : '') . $description : '');Maybe the next example would be more clean, but with more function calls. I don't know if performance is relevant here? $return = [];
$return[] = $this->type ? $this->type->__toString() : '';
$return[] = $this->variableName ? ($this->isReference ? '&' : '') . ($this->isVariadic ? '...' : '') . '$' . $this->variableName : '';
$return[] = $this->description ? $this->description->render() : '';
return implode(' ', array_filter($return, fn($v) => ($v !== ''))); |
|
My example was a simplified version. |
Build errors:
⇾ https://travis-ci.org/github/JetBrains/phpstorm-stubs/builds/723069982
Pull request where I try to add "&" and "..." in the docs:
⇾ JetBrains/phpstorm-stubs#892
This change is