search

PHP-CS-Fixer / PHP-CS-Fixer

A tool to automatically fix PHP Coding Standards issues
https://cs.symfony.com
MIT License
12.81k stars 1.58k forks source link

Match -> switch rule and fixer #8084

Closed uuf6429 closed 3 months ago

uuf6429 commented 3 months ago

Feature request

The main reason is that PHPUnit (or, as I understand, test coverage in general) cannot properly cover match expressions.

The motivation behind such a rule is that:

  1. I can fix the already mentioned problem in the current codebase with a fixer rule
  2. I can (somewhat) clearly convey to collaborators that that rule is in effect (without me having to write a "please do not refactor this switch into a match because ...." comment on every switch).

Note that the opposite rule/fixer was being discussed here: https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/issues/5894

I might try contributing such a rule when provided some starting point (e.g. some past PR that I could follow).

Link to Discussion where feature request was formed

No response

Contribution Checks

keradus commented 3 months ago

Hey, thanks for sharing the proposal!

I decided to not accept this idea for core repository. While I understand the downside of non-accurate code-coverage report, I believe it's better to have slightly inaccurate code coverage, than moving runtime from strict comparison (match) to loose one (switch). And for code quality metrics, I suggest to enrich code-coverage for lines with mutation score (eg infection), that will lead to further code quality guards.

If you want to have such rule, you may create a custom fixers.

uuf6429 commented 3 months ago

better to have slightly inaccurate code coverage

Marking an entire match statement (including all branches) as covered (even when only one has been so) is wildly inaccurate and extremely misleading.

than moving runtime from strict comparison (match) to loose one (switch).

I'd have avoided that by have a strictness check in the branch expression e.g. match ($x) { 1 => ... => switch (true) { case $x === 1: ...

enrich code-coverage for lines with mutation score

I've read the suggestions from linked PHPUnit issue :) The problem is that I don't want to introduce another tool just for match (and even less somehow hack the result of that tool to have a unified coverage report).

The other reason this rule could/would make sense is for the simple fact that beyond simple cases, (that could have been implemented with a hashmap), match expressions are a bit of an eyesore (compared to switch).

If you want to have such rule, you may create a custom fixer.

Fine, I'll consider that approach.