Skip to content

Use TypeUtils::getOldConstantArrays in array_column extension#1683

Merged
ondrejmirtes merged 1 commit intophpstan:1.8.xfrom
herndlm:array-column-get-old-constant-arrays
Sep 1, 2022
Merged

Use TypeUtils::getOldConstantArrays in array_column extension#1683
ondrejmirtes merged 1 commit intophpstan:1.8.xfrom
herndlm:array-column-get-old-constant-arrays

Conversation

@herndlm
Copy link
Copy Markdown
Contributor

@herndlm herndlm commented Sep 1, 2022

looks like it made it a tiny bit smarter. testImprecise4 was converted to testConstantArray12

@ondrejmirtes
Copy link
Copy Markdown
Member

BTW if you don't plan to make more changes and if you're not just experimenting, you don't need to open the PR as a draft. The problem is that I don't get an email notification when you mark it as ready for review.

if you're just waiting for CI results, you can open it as ready-to-merge from the start. If the CI looks green I can merge it sooner.

@herndlm
Copy link
Copy Markdown
Contributor Author

herndlm commented Sep 1, 2022

Good to know. Yeah, just always anxious that ci might be red. Like here :)

@herndlm herndlm force-pushed the array-column-get-old-constant-arrays branch from 9e47876 to dc4a54b Compare September 1, 2022 11:21
@herndlm herndlm marked this pull request as ready for review September 1, 2022 11:27
@herndlm
Copy link
Copy Markdown
Contributor Author

herndlm commented Sep 1, 2022

how do you wish to have the new Type::getConstantArrays() implemented? :)
return an empty array everywhere there are none directly or via trait? UPDATE: maybe I should study existing traits first..

@ondrejmirtes
Copy link
Copy Markdown
Member

Yeah, ConstantArrayType should return [$this], UnionType should merge the results from all the types.

IntersectionType I'm not sure about, you can have ConstantArrayType&NonEmptyArrayType...

@ondrejmirtes
Copy link
Copy Markdown
Member

And please search and replace the usages programatically, either with a regex in PhpStorm, or write a script that uses PhpParser format-preserving printer and just switch those AST nodes around in a node visitor :) Here's an example: https://github.com/nikic/PHP-Parser/blob/master/doc/component/Pretty_printing.markdown#formatting-preserving-pretty-printing

@herndlm
Copy link
Copy Markdown
Contributor Author

herndlm commented Sep 1, 2022

Yeah, ConstantArrayType should return [$this], UnionType should merge the results from all the types.

IntersectionType I'm not sure about, you can have ConstantArrayType&NonEmptyArrayType...

right, but I meant mostly how e.g. IntegerType should implement it. I wonder if I should create a new e.g. NotConstantArrayType trait or just implement it returning an empty array in most of the types

@ondrejmirtes
Copy link
Copy Markdown
Member

Both are valid to me :) I don't like the traits too much but we already accepted that direction so it's fine for me.

@herndlm
Copy link
Copy Markdown
Contributor Author

herndlm commented Sep 1, 2022

fyi I have the Type::getConstantArrays adaption ready and will open the PR after this one got merged :)

@ondrejmirtes ondrejmirtes merged commit 03a76b7 into phpstan:1.8.x Sep 1, 2022
@ondrejmirtes
Copy link
Copy Markdown
Member

Alright, thank you :)

@herndlm herndlm deleted the array-column-get-old-constant-arrays branch September 1, 2022 12:37
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