Skip to content

Support list<Foo>#79

Closed
orklah wants to merge 14 commits intophpstan:masterfrom
orklah:list-type-implement
Closed

Support list<Foo>#79
orklah wants to merge 14 commits intophpstan:masterfrom
orklah:list-type-implement

Conversation

@orklah
Copy link
Copy Markdown
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
Copy Markdown
Member

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

@ondrejmirtes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Otherwise perfect! Thank you.

@ondrejmirtes
Copy link
Copy Markdown
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
Copy Markdown
Member

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

@orklah
Copy link
Copy Markdown
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants