Skip to content

progress on named argument helper#1313

Merged
ondrejmirtes merged 8 commits into
phpstan:1.7.xfrom
staabm:named-helper
May 15, 2022
Merged

progress on named argument helper#1313
ondrejmirtes merged 8 commits into
phpstan:1.7.xfrom
staabm:named-helper

Conversation

@staabm

@staabm staabm commented May 14, 2022

Copy link
Copy Markdown
Contributor

implement details mentioned in #1305 (comment) and remaining parts from #1301 (comment)

@staabm

staabm commented May 14, 2022

Copy link
Copy Markdown
Contributor Author

regarding

And similarly, if someone calls foo(1, 2) (named arguments were not used, but some required arguments are missing), we can also return null to prevent some nasty bugs in the extensions.

atm the helper returns the call-args as is, since the helper only cares about args containing named-args.
I don't see this requirement related to the named-arguments-helper - isn't this case already handled somewhere within phpstan-core?

just added a test to demonstrate the helper returns the call unchanged.


I think the helper is mostly complete as requested. next question would be on how to handle the returned null value. should it just be turned into a ErrorType on the calling sides?

@ondrejmirtes

ondrejmirtes commented May 14, 2022

Copy link
Copy Markdown
Member

This is what I meant: c7969d0?diff=unified&w=1

There's still one test failure, please fix it. And I guess it's still not called for all relevant extension types, right?

I don't see this requirement related to the named-arguments-helper

I plan to rename the class to "ArgumentsNormalizer" in the next step so the name will no longer lie about the purpose.

@staabm

staabm commented May 14, 2022

Copy link
Copy Markdown
Contributor Author

And similarly, if someone calls foo(1, 2) (named arguments were not used, but some required arguments are missing), we can also return null to prevent some nasty bugs in the extensions.

I have this change prepared locally but it still has some issues. at best we can merge here and progress in a new PR..?

@staabm staabm marked this pull request as ready for review May 14, 2022 20:58
@ondrejmirtes ondrejmirtes merged commit 4d584d1 into phpstan:1.7.x May 15, 2022
@ondrejmirtes

Copy link
Copy Markdown
Member

at best we can merge here and progress in a new PR..?

Yes :) Thank you!

@staabm staabm deleted the named-helper branch May 15, 2022 22: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.

3 participants