Skip to content
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.

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

test: Improve ExplicitIndirectVariableFixerTest #7451

Conversation

Wirone
Copy link
Member

@Wirone Wirone commented Nov 21, 2023

  • add more test cases
  • add yield keys (better --testdox output)
  • add missing iterable type for PHPStan

Related to #7442.

@Wirone Wirone self-assigned this Nov 21, 2023
@coveralls
Copy link

coveralls commented Nov 21, 2023

Coverage Status

coverage: 94.795%. remained the same
when pulling 1f3e726 on Wirone:codito/improve-explicit_indirect_variable-test-cases
into f34d12c on PHP-CS-Fixer:master.

@Wirone Wirone requested a review from keradus November 22, 2023 09:36
@Wirone Wirone force-pushed the codito/improve-explicit_indirect_variable-test-cases branch from 935bb34 to d5b086d Compare November 22, 2023 09:36
@@ -33,29 +33,32 @@ public function testFix(string $expected, ?string $input = null): void
$this->doTest($expected, $input);
}

/**
* @return iterable<string, array{0: string, 1?: null|string}>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @return iterable<string, array{0: string, 1?: null|string}>
* @return iterable<string, array{0: string, 1?: string}>

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically it's possible to return null explicitly as a second array item and it will work, because testFix signature allows it. I just reflected the actual contract 🙂.

Copy link
Member

Choose a reason for hiding this comment

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

it's technically possible to provide null to testFix, but I wouldn't like to have provider explicitly returning null.

IMHO we shall have same approaches for all providers, i was always using 1?: string. if you want to change approach, please change for whole project.

but again, i would avoid provider returning null, either 0: string, 1?: string or 0: string, 1: null|string. combining ? with |null is overkill and bringing more than one way to provide data cases

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, but it's forward-compatible with providers returning more than 2 elements, e.g. rules configuration. I am pretty sure there are already places where input is explicitly returned as null because of that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@keradus the example: signature + test case with explicit null as the input value. I know it can be changed later if third argument is introduced, but I think it's good to describe existing contract from the beginning.

Copy link
Contributor

@kubawerlos kubawerlos left a comment

Choose a reason for hiding this comment

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

LGTM, the discussion about the PHPDoc is not really related to the topic of this PR, I believe we can shift such discussion to separate conversations.

@Wirone Wirone force-pushed the codito/improve-explicit_indirect_variable-test-cases branch from d5b086d to 1f3e726 Compare November 23, 2023 21:03
@Wirone Wirone enabled auto-merge (squash) November 23, 2023 21:04
@Wirone Wirone merged commit a7fa115 into PHP-CS-Fixer:master Nov 23, 2023
23 checks passed
@Wirone Wirone deleted the codito/improve-explicit_indirect_variable-test-cases branch November 23, 2023 21:15
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.

None yet

4 participants