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 support for url() helper method #179

Merged
merged 2 commits into from
Oct 30, 2018
Merged

Add support for url() helper method #179

merged 2 commits into from
Oct 30, 2018

Conversation

devfrey
Copy link
Contributor

@devfrey devfrey commented Oct 24, 2018

Fixes #177

Since there were no unit tests covering the existing return type helpers, I gave it my best shot to write my own tests. Please let me know what you think.

Notes
This implementation only covers usage like this:

$generator = url();

In theory, passing null as the first argument (directly or indirectly) would also result in a UrlGenerator instance, but I was not able to implement a check for that.

@devfrey
Copy link
Contributor Author

devfrey commented Oct 24, 2018

What's up with StyleCI's strange conventions for imports?

(analyses: https://github.styleci.io/analyses/q2RoxB)

@szepeviktor
Copy link
Collaborator

Very nice job.

@Korri
Copy link

Korri commented Oct 25, 2018

Nice PR! For the Tests though, you can get away with a bit less mocking (which is always better to prevent changes in PHPStan from breaking the tests), have a look at how we did it here: https://github.com/lxrco/phpstan-laravel/blob/master/tests/HelpersReturnTypeExtensionTest.php

@devfrey
Copy link
Contributor Author

devfrey commented Oct 25, 2018

Nice PR! For the Tests though, you can get away with a bit less mocking

I see the same amount of mocked objects in the test you referenced. Am I missing something? The only (big) difference is the use of a data provider, which I didn't find suitable because there are different assertions for both return types. (In case of ObjectType it also performs an assertion on the object type, which is not the case for the string return type).

@szepeviktor
Copy link
Collaborator

szepeviktor commented Oct 25, 2018

And maybe the copyright needs changing: https://travis-ci.org/nunomaduro/larastan/jobs/445860198#L674

@devfrey
Copy link
Contributor Author

devfrey commented Oct 25, 2018

@szepeviktor I've added the copyright notice. Perhaps consider mentioning that requirement in Contributing?

@szepeviktor
Copy link
Collaborator

Okay.
Now all should be green!

Copy link
Collaborator

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

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

This Pull Request is perfect. The only thing that should be discussed: Testing should be addressed like this in Larastan?

I would prefer a feature testing instead of unit testing. Idea: Having a Laravel Project on the tests folder with the following code:

echo url('bla'); and url()->bla().

The test would be running Larastan on the level max and if no errors happen then the tests passes.

Any feedback?

@devfrey
Copy link
Contributor Author

devfrey commented Oct 26, 2018

Thanks, @nunomaduro.

I agree with you – I would also prefer behavioural testing over unit testing. Perhaps something similar to php-src's tests. Special files with PHP code in them, and also the expected (in this case PHPStan) output. But I'm not sure how easy that would be to implement.

@nunomaduro
Copy link
Collaborator

It’s easy, I will work on an example tonight ok? 🤙

@nunomaduro
Copy link
Collaborator

@devfrey Just added tests for the view and response helpers. Can you add tests for the url extension?

@devfrey
Copy link
Contributor Author

devfrey commented Oct 30, 2018

@devfrey Just added tests for the view and response helpers. Can you add tests for the url extension?

Sure thing!

The return type of the url() helper method is determined by the value of the first method parameter. If this parameter is null, an instance of UrlGenerator is returned. Otherwise, it returns a string.
@devfrey
Copy link
Contributor Author

devfrey commented Oct 30, 2018

I've added new tests!

@devfrey devfrey closed this Oct 30, 2018
@devfrey devfrey reopened this Oct 30, 2018
@nunomaduro nunomaduro merged commit aa21727 into larastan:master Oct 30, 2018
@nunomaduro
Copy link
Collaborator

Thanks for this @devfrey !

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

4 participants