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 extension for app return type, including test cases. #431

Merged
merged 9 commits into from
Feb 4, 2020

Conversation

troelsselch
Copy link
Contributor

@troelsselch troelsselch commented Jan 28, 2020

I have added a simple return type extension for the app() helper.
Fully qualified classes will be resolved to the class itself. Unknown types, e.g. 'sentry' or 'db', will be resolved to MixedType.

This will allow checking for method calls directly on app(), so that e.g.

/** @var Handler $handler */
$handler = app(Handler::class);
$handler->capture($exception);

can be simplified to

app(Handler::class)->capture($exception);

and still be verified by larastan.

@nunomaduro
Copy link
Collaborator

I don't see a reason for having a MixedType here. You should resolve things out of the container, and return the type of the resolved value.

Copy link
Collaborator

@canvural canvural left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Thank you for the PR! I left some comments.

src/ReturnTypes/Helpers/AppExtension.php Outdated Show resolved Hide resolved
src/ReturnTypes/Helpers/AppExtension.php Outdated Show resolved Hide resolved
src/ReturnTypes/Helpers/AppExtension.php Outdated Show resolved Hide resolved
src/ReturnTypes/Helpers/AppExtension.php Outdated Show resolved Hide resolved
@troelsselch
Copy link
Contributor Author

Thanks for the feedback. I have cleaned up the code and try to resolve String_ values from the container.

I still have two cases with MixedType():

  • if $expr is not instance of ClassConstFetch, and
  • if $expr->class is instance of Name

In the first case I cannot see another option.
In the latter case I could process the $expr->class and $expr->name and try to resolve that in the container. However, that requires more reflection checking in case class is e.g. self.

What are your thoughts on this?

@canvural canvural self-requested a review January 29, 2020 10:06
@troelsselch troelsselch force-pushed the feature-AppExtension branch 2 times, most recently from a12828f to 7a0248b Compare January 31, 2020 16:32
src/ReturnTypes/Helpers/AppExtension.php Outdated Show resolved Hide resolved
src/ReturnTypes/Helpers/AppExtension.php Outdated Show resolved Hide resolved
src/ReturnTypes/Helpers/AppExtension.php Show resolved Hide resolved
src/ReturnTypes/Helpers/AppExtension.php Outdated Show resolved Hide resolved
@troelsselch
Copy link
Contributor Author

troelsselch commented Feb 4, 2020

I have rebased and updated to be using ErrorType and NeverType as suggested.

@canvural canvural merged commit 4ccdaf9 into larastan:master Feb 4, 2020
@canvural
Copy link
Collaborator

canvural commented Feb 4, 2020

Thank you @troelsselch ! Good first contribution!

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.

3 participants