Support string arguments #16

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

Jump to bottom

Closed

lookyman wants to merge 2 commits into phpstan:master from lookyman:string-arg-support

Closed

Support string arguments #16

lookyman wants to merge 2 commits into phpstan:master from lookyman:string-arg-support

Conversation

@lookyman

Copy link

Contributor

@lookyman lookyman commented Feb 18, 2018 •

edited

Loading

No description provided.

@lookyman lookyman force-pushed the string-arg-support branch from cfcd5c9 to 938e4ea Compare

February 18, 2018 10:54

ondrejmirtes

ondrejmirtes requested changes

Feb 18, 2018

View reviewed changes

src/Rules/PHPUnit/AssertSameBooleanExpectedRule.php Outdated

'You should use assertFalse() instead of assertSame() when expecting "false"',

];

if ($leftType instanceof ConstantBooleanType) {

if ($leftType->getValue() === true) {

Copy link

Member

@ondrejmirtes ondrejmirtes Feb 18, 2018

Choose a reason for hiding this comment

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

No need to compare against === true, it's boolean itself.

src/Type/PHPUnit/CreateMockDynamicReturnTypeExtension.php Outdated

$class = $arg->class;

if (!($class instanceof \PhpParser\Node\Name)) {

$class = (string) $class;

} elseif ($arg instanceof \PhpParser\Node\Scalar\String_) {

Copy link

Member

@ondrejmirtes ondrejmirtes Feb 18, 2018

Choose a reason for hiding this comment

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

Please update this with the latest advancements in PHPStan's typesystem. See phpstan/phpstan@a46cbc0 for an example.

Please also move the fixes of changes in PHPStan's dev-master to a separate commit.

Copy link

Contributor Author

@lookyman lookyman Feb 18, 2018

Choose a reason for hiding this comment

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

done

@lookyman lookyman force-pushed the string-arg-support branch 2 times, most recently from 6186e99 to 039c23e Compare

February 18, 2018 12:27

ondrejmirtes

ondrejmirtes requested changes

Feb 18, 2018

View reviewed changes

Copy link

Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

More changes :)

src/Type/PHPUnit/CreateMockDynamicReturnTypeExtension.php Outdated

if ($argType instanceof TypeWithClassName) {

$class = $argType->getClassName();

} elseif ($argType instanceof ConstantStringType) {

$class = $argType->getValue();

Copy link

Member

@ondrejmirtes ondrejmirtes Feb 18, 2018

Choose a reason for hiding this comment

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

You didn't add any tests for this.

Copy link

Contributor Author

@lookyman lookyman Feb 18, 2018

Choose a reason for hiding this comment

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

There were no tests for this before, that's why. I'll write some asap.

src/Type/PHPUnit/CreateMockDynamicReturnTypeExtension.php Outdated

$class = $arg->class;

if (!($class instanceof \PhpParser\Node\Name)) {

$argType = $scope->getType($methodCall->args[$argumentIndex]->value);

if (!($argType instanceof TypeWithClassName)) {

Copy link

Member

@ondrejmirtes ondrejmirtes Feb 18, 2018

Choose a reason for hiding this comment

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

This doesn't look right to me - Foo::class does not result in TypeWithClassName but in ConstantStringType. Also, this does not break on dev-master so I don't see a reason to change it.

Copy link

Contributor Author

@lookyman lookyman Feb 18, 2018

Choose a reason for hiding this comment

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

And you are right of course. Will fix that.

@lookyman


 Update for latest PHPStan master

a773786

@lookyman lookyman force-pushed the string-arg-support branch 3 times, most recently from e2fd5c4 to 5bc9a47 Compare

February 19, 2018 19:47

@lookyman


 Support string arguments

5c52578

@lookyman lookyman force-pushed the string-arg-support branch from 5bc9a47 to 5c52578 Compare

February 20, 2018 08:51

@lookyman

Copy link

Contributor Author

lookyman commented Feb 20, 2018

Ok, how about now?

@ondrejmirtes

Copy link

Member

ondrejmirtes commented Feb 21, 2018

Can you please open a PR with ExtensionTestCase in phpstan/phpstan first? Thank you.

@lookyman

Copy link

Contributor Author

lookyman commented Feb 21, 2018

Sure, no problem. Should NodeScopeResolverTest extend it? And if yes, maybe it shouldn't be named ExtensionTestCase.. Can you suggest a better name?

@ondrejmirtes

Copy link

Member

ondrejmirtes commented Feb 21, 2018 via email

I’d rather not use inheritance. Make a class under PHPStan\Testing and register it as a service. The tests can get it from the DI container.

On 2018年2月21日 at 15:03, Lukáš Unger ***@***.***> wrote: Sure, no problem. Should NodeScopeResolverTest extend it? And if yes, maybe it shouldn't be named ExtensionTestCase.. Can you suggest a better name? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#16 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGZuAMJ2ao_3CV5xizLNwalk8XD3t9hks5tXCJKgaJpZM4SJost> .

-- Ondřej Mirtes

@lookyman

Copy link

Contributor Author

lookyman commented Apr 10, 2018

I believe this is already irrelevant? Feel free to close this.

@ondrejmirtes

Copy link

Member

ondrejmirtes commented Apr 10, 2018

Yep, right, thanks :)

@ondrejmirtes ondrejmirtes closed this

Apr 10, 2018

Labels

None yet

2 participants

@lookyman @ondrejmirtes

Support string arguments #16

Support string arguments #16

Uh oh!

Conversation

@lookyman lookyman commented Feb 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@ondrejmirtes ondrejmirtes Feb 18, 2018

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes Feb 18, 2018

Choose a reason for hiding this comment

Uh oh!

@lookyman lookyman Feb 18, 2018

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes Feb 18, 2018

Choose a reason for hiding this comment

Uh oh!

@lookyman lookyman Feb 18, 2018

Choose a reason for hiding this comment

Uh oh!

@ondrejmirtes ondrejmirtes Feb 18, 2018

Choose a reason for hiding this comment

Uh oh!

@lookyman lookyman Feb 18, 2018

Choose a reason for hiding this comment

Uh oh!

lookyman commented Feb 20, 2018

Uh oh!

ondrejmirtes commented Feb 21, 2018

Uh oh!

lookyman commented Feb 21, 2018

Uh oh!

ondrejmirtes commented Feb 21, 2018 via email

Uh oh!

lookyman commented Apr 10, 2018

Uh oh!

ondrejmirtes commented Apr 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

@lookyman lookyman commented Feb 18, 2018 •

edited

Loading