Skip to content

Conversation

orklah
Copy link
Contributor

@orklah orklah commented Jan 3, 2020

As discussed in phpstan/phpstan/issues/2788, this PR implements the notations
list and list<Foo> as aliases of respectively array<int, mixed> and array<int, Foo>

I added some tests on ReturnTypeRuleTest.php but I'm not sure it was the best place to put them. Please tell me if it shoud be located elsewhere or if it's missing tests.

Also, should I try to add some tests in https://github.com/phpstan/phpdoc-parser to ensure those notations will stay stable?

@ondrejmirtes
Copy link
Member

Type inference is best tested like this (the part with tests/PHPStan/Analyser/NodeScopeResolverTest.php): 65e578c#diff-b5a18b99fdc03e387e605f09db47dc15

@ondrejmirtes
Copy link
Member

Also, should I try to add some tests in https://github.com/phpstan/phpdoc-parser to ensure those notations will stay stable?

No, isn't necessary.

{
public function withoutGenerics(): void
{
/** @var list $list */
Copy link
Member

Choose a reason for hiding this comment

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

You should at least once typehint the type as method parameter and assert it right at the beginning of the body. If you assert the variable only after some changes (array appending), you have no guarantees about the state right after $list = [];.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I added some test cases with direct assertions. It should be ok

@ondrejmirtes
Copy link
Member

Otherwise perfect! Thank you.

@ondrejmirtes
Copy link
Member

Please get rid of the merge commits and rebase it onto master. I'm not able to wrap my head around this Git history. Thank you.

@ondrejmirtes
Copy link
Member

It'd probably be easiest to start a new branch and just do all the changes in a single commit.

@orklah
Copy link
Contributor Author

orklah commented Jan 3, 2020

It'd probably be easiest to start a new branch and just do all the changes in a single commit.

Yeah, I'll probably do that. I'm not really an expert with git!

@orklah orklah mentioned this pull request Jan 3, 2020
@orklah orklah deleted the list-type-implement branch January 4, 2020 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants