Skip to content

minor: NativeFunctionTypeDeclarationCasingFixer - Add null and false support #6427

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.

Sign up for GitHub

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
wants to merge 1 commit into from
Closed

minor: NativeFunctionTypeDeclarationCasingFixer - Add null and false support #6427

wants to merge 1 commit into from

Conversation

SpacePossum
Copy link
Contributor

@SpacePossum SpacePossum commented Jun 13, 2022 •
edited by julienfalque
Loading

Add missing support for null and false

@coveralls
Copy link

coveralls commented Jun 13, 2022 •
edited
Loading

Coverage Status

Coverage decreased (-0.004%) to 92.912% when pulling bc2eb76 on SpacePossum:master_NativeFunctionTypeDeclarationCasingFixer_false_null_support into aafad72 on FriendsOfPHP:master.

@julienfalque julienfalque changed the title minor: NativeFunctionTypeDeclarationCasingFixer - Add null and false sup… minor: NativeFunctionTypeDeclarationCasingFixer - Add null and false support Jul 5, 2022
@SpacePossum SpacePossum force-pushed the master_NativeFunctionTypeDeclarationCasingFixer_false_null_support branch from 7c760b1 to 3af0201 Compare August 31, 2022 09:21
@SpacePossum
Copy link
Contributor Author

rebased on latest, no other changes

@SpacePossum SpacePossum mentioned this pull request Sep 2, 2022
Closed
1 task
@Wirone
Copy link
Member

Wirone commented Sep 2, 2022

Can we support also true standalone type here? It would cover #6605 🙂

@SpacePossum
Copy link
Contributor Author

SpacePossum commented Sep 2, 2022 •
edited
Loading

yeah sure @Wirone , thanks for pointing it out! If you have time and want to take a try at it please send a PR to my fork :)
if not all good, than I'll pick this is up as soon as I've time

@Wirone Wirone mentioned this pull request Sep 4, 2022
Merged
@Wirone
Copy link
Member

Wirone commented Sep 4, 2022 •
edited
Loading

@SpacePossum I'll be glad if you do fast-forward merge and include my commits in this PR 😉

I think you should do the rebase in order to use #6558, I'll do rebase of SpacePossum#1 then too.

kubawerlos
kubawerlos reviewed Sep 4, 2022
View reviewed changes
@Wirone Wirone mentioned this pull request Sep 5, 2022
Merged
@SpacePossum SpacePossum force-pushed the master_NativeFunctionTypeDeclarationCasingFixer_false_null_support branch from ed3b948 to 68a1f8a Compare September 5, 2022 09:19
Wirone
Wirone reviewed Sep 5, 2022
View reviewed changes
tests/Fixer/Casing/NativeFunctionTypeDeclarationCasingFixerTest.php Outdated
Comment on lines 219 to 220
sprintf('<?php class T { public function Foo(%s $A): %1$s {return %1$s;}}', $type),
sprintf('<?php class T { public function Foo(%s $A): %1$s {return %1$s;}}', strtoupper($type)),
Copy link
Member

Choose a reason for hiding this comment

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

IMHO:

  • it's not readable enough, would be better with explicit null
  • return null; is not required here because null here is not a type, it's a scalar value. And that's why tests are failing because NativeFunctionTypeDeclarationCasingFixer is related to types so it's not its responsibility to fix values' casing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks updated 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you want, feel free to take over this PR, I'm juggling to many PR's already 😅 and I think you know more about this change in PHP8.2

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I can do it, but how to do it technically? Should I take this branch to my fork and open new MR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, or on your fork make a new branch from latest, than cherrypick the one commit from here (I already squashed the commits), than make a new PR and we should be good :)

Copy link
Member

Choose a reason for hiding this comment

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

@SpacePossum OK, I've created #6623 🙂

@SpacePossum SpacePossum force-pushed the master_NativeFunctionTypeDeclarationCasingFixer_false_null_support branch from c1fa861 to bc2eb76 Compare September 5, 2022 12:15
@Wirone Wirone mentioned this pull request Sep 5, 2022
Merged
@SpacePossum
Copy link
Contributor Author

will be finalized in #6623 , thanks all reviewers 👍

@SpacePossum SpacePossum closed this Sep 5, 2022
@SpacePossum SpacePossum deleted the master_NativeFunctionTypeDeclarationCasingFixer_false_null_support branch October 3, 2022 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wirone Wirone Wirone left review comments

@julienfalque julienfalque julienfalque left review comments

@keradus keradus keradus left review comments

@kubawerlos kubawerlos kubawerlos left review comments

Assignees
No one assigned
Labels
kind/enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@SpacePossum @coveralls @Wirone @julienfalque @keradus @kubawerlos