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

feat: added better typing for the Optional class and helper. #1344

Merged
merged 1 commit into from
Oct 10, 2022

Conversation

mad-briller
Copy link
Contributor

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Changes

Added a conditional return type to the optional() method along with template typing.
Made the Optional class a universalObjectCrateClass as it will return a value for all property names.

@canvural
Copy link
Collaborator

Can you rebase it on top of master branch instead of a merge commit? Thanks!

@canvural canvural changed the title Added better typing for the Optional class and helper. feat: added better typing for the Optional class and helper. Oct 10, 2022
@canvural canvural merged commit 52448d7 into larastan:master Oct 10, 2022
@canvural
Copy link
Collaborator

Thank you!

@canvural
Copy link
Collaborator

Hi @mad-briller

This change caused some errors in my testing.

For example now optional($this->some_carbon_date_time_field)->toIso8601String() produces the error Call to an undefined method Illuminate\Support\Optional::toIso8601String()

Could you take a look at that? Thanks.

@mad-briller
Copy link
Contributor Author

i guess in the past this wasn't raised because of the explicit mixed present in laravel's phpdoc for optional(), now that phpstan knows it's Optional it's trying to make more guarantees about it

it may take a bit of work for me to do that and i'm not sure when i can get to it, my idea would be to make Optional generic and then use some property / method reflections to see if the proxied object has that property / method

@canvural
Copy link
Collaborator

Sure no problem. I also like the generics idea. I might look into that tonight!

@mad-briller
Copy link
Contributor Author

oh sweet, i'll keep my eye out, thanks

if we do make Optional generic, the universalObjectCratesClasses definition for Optional can be removed, because the property reflection would want to take precedence and can return new MixedType in the case it is not present

@canvural
Copy link
Collaborator

@mad-briller On a second though I decided not to go generics way. There is no reason to complicate things. Since ?-> exists in PHP using optional is not needed anymore anyway. So I did this 8cdd75d We will still get better types when callback is used. But for calls just with the value, we assume mixed

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.

None yet

3 participants