Skip to content
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

Open
wants to merge 12 commits into
base: 1.x
Choose a base branch
from
Open

Add new types #211

wants to merge 12 commits into from

Conversation

ahjdev
Copy link

@ahjdev ahjdev commented Jul 20, 2024

I tried to add new types except class-string-map<T of Foo, T> which is not handled by phpstan
I also want to resolve template tags (which is I am wonder how?)

@jaapio
Copy link
Member

jaapio commented Jul 22, 2024

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?

@ahjdev
Copy link
Author

ahjdev commented Jul 23, 2024

Well I did not know about extending underlying type (Which is will be my next commit)

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.

About that, well I did not get it. For example you mean OpenResource must not be final?

@ahjdev
Copy link
Author

ahjdev commented Jul 23, 2024

@ahjdev
Copy link
Author

ahjdev commented Jul 23, 2024

Also I think I should just remove AggregatedPseudoType and extend IntMask and IntMaskOf from AggregatedType. (Well I just think ArrayShape need to review too)

@jaapio
Copy link
Member

jaapio commented Jul 23, 2024

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.

@ahjdev
Copy link
Author

ahjdev commented Jul 24, 2024

I would appreciate if you start workflow for my last commit

@ahjdev
Copy link
Author

ahjdev commented Jul 27, 2024

I was checking classes and I noticed it is impossible to a property with type of ?Type became null.
Because createType method will return Mixed_ class for null

@jaapio
Copy link
Member

jaapio commented Aug 14, 2024

Reason to never use null is because everything has a type. I do see Mixed_ as an unknown or undefined type. Where php does have null as possible types in this library we do wrap nullable types in Nullable

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.

@ahjdev
Copy link
Author

ahjdev commented Aug 16, 2024

I can covert tests too. just how?

@ahjdev
Copy link
Author

ahjdev commented Nov 22, 2024

I am still waiting for merge this pr.
Also why ReflectionDocBlock doesn't use latest version of TypeResolver?

@jaapio
Copy link
Member

jaapio commented Nov 22, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants