-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add new types #211
base: 1.x
Are you sure you want to change the base?
Add new types #211
Conversation
Thanks for your pr, it would be useful to split this PR into several smaller PRS. so we can discuss the individual additions you did. Some of them can be merged right away or with a few small changes. Other changes might need more discussion. A good starting point could be the commits you did in separate prs? A first comment regarding the changes you did. The pseudotypes should extend the underlying type. I see that this wasn't properly applied to all pseudotypes. If the original class is final we can remove the final keyword. So for example the OpenResource type extends the Resource type. And implements the pseudo type. I think the condional type should extend the mixed type. Or maybe a union type? This would make it easier for consuming libraries to ignore the condition. And it will still reflect something useful? |
Well I did not know about extending underlying type (Which is will be my next commit)
About that, well I did not get it. For example you mean OpenResource must not be final? |
https://github.com/phpDocumentor/TypeResolver/blob/1.x/src/PseudoTypes/FloatValue.php Take a look. it did not extend too. |
Also I think I should just remove |
I get your confusion. And when looking for an example I discovered that I didn't do this myself correctly in the past. The pseudotypes were introduced with the True and False class. You can use those as an example. |
I would appreciate if you start workflow for my last commit |
I was checking classes and I noticed it is impossible to a property with type of |
Reason to never use What blocks me from merging this PR is that we really need tests. You did introduce quite a number of new types which is very nice and I'm grateful you did that. But without tests I cannot pass this. Apart from that, thank you very much for all the work you already did. |
I can covert tests too. just how? |
I am still waiting for merge this pr. |
I'm missing tests for the added types, to move forward faster I think it's better to split this pr in to several smaller ones, so I can merge and review the types individual. Right now this is a very big change without tests. That's what holding me back. Based on your last comment I expected you were still working on this. ReflectionDocBlock is able to work with newer and older versions of this library, so that's why it hasn't been changed |
I tried to add new types except
class-string-map<T of Foo, T>
which is not handled by phpstanI also want to resolve template tags (which is I am wonder how?)