Skip to content

add T_MATCH to increments #8

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

Merged
merged 9 commits into from
Mar 22, 2023
Merged

Conversation

epic-64
Copy link
Contributor

@epic-64 epic-64 commented Mar 22, 2023

Subject:
I would like to use this package to report cognitive complexity.
In my codebase I use match() a lot, which was introduced in PHP 8.0.
It is similar to switch and from what I can tell, should get the same treatment.

Changes:

  • added T_MATCH to increments in src/CognitiveComplexity/Analyzer.php
  • added test for match construct in tests/Data/function11.php.inc
  • increased minimum required version of squizlabs/php_codesniffer to 3.6.0

Additional changes (necessary):

  • lift dependency of PHP version from ^7.2 to ^8.0 (where T_MATCH is introduced)

Additional changes (optional):

  • Added a Dockerfile and docker-compose.yml for easy local testing (using composer image)
  • Added phpunit to dev-dependencies for local testing
  • Applied suggested changes to phpunit.xml using vendor/bin/phpunit tests --migrate-configuration

@jrfnl
Copy link

jrfnl commented Mar 22, 2023

lift dependency of PHP version from ^7.2 to ^8.0 (where T_MATCH is introduced)

That sound completely unnecessary to me. PHP_CodeSniffer backfills the T_MATCH token since PHPCS 3.6.0 (all the way back to PHP 5.4). So the only thing needed would be to raise the minimum PHPCS version to 3.6.0 (or later).

Applied suggested changes to phpunit.xml using vendor/bin/phpunit tests --migrate-configuration

Not a good idea as that will prevent the tests from running on PHPUnit < 9.3 and with a PHP minimum of PHP 7.2, PHPUnit 8.x is still needed.

@epic-64 epic-64 changed the title update to php 8.0, add T_MATCH equivalent to T_SWITCH draft: update to php 8.0, add T_MATCH equivalent to T_SWITCH Mar 22, 2023
@epic-64 epic-64 marked this pull request as draft March 22, 2023 17:03
@epic-64 epic-64 changed the title draft: update to php 8.0, add T_MATCH equivalent to T_SWITCH update to php 8.0, add T_MATCH equivalent to T_SWITCH Mar 22, 2023
@epic-64 epic-64 marked this pull request as ready for review March 22, 2023 17:17
@epic-64
Copy link
Contributor Author

epic-64 commented Mar 22, 2023

I was not aware of the backwards compatibility in PHPCS. I removed the unnecessary upgrades

@epic-64 epic-64 changed the title update to php 8.0, add T_MATCH equivalent to T_SWITCH add T_MATCH to increments Mar 22, 2023
Copy link
Owner

@Rarst Rarst left a comment

Choose a reason for hiding this comment

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

  1. Please remove Docker files, I am not sure why would they be necessary or relevant to the PR.
  2. I've approved the test to run for PR and setup is currently crashing over PHPUnit 9 being required in the changed lock and not compatible with composer.json.
  3. Actually please drop requiring PHPUnit. It's not relevant to the enhancement and I don't have it in me to ponder changes to the setup right now.

@epic-64
Copy link
Contributor Author

epic-64 commented Mar 22, 2023

Adding PHPUnit and the dockerfiles were necessary for me to get anything working locally, specifically composer install and phpunit tests.

I did now add the docker files to my excludes so that they are no longer added to the repo. As for PHPunit, I removed that but can no longer test locally. I suppose there is some way to install and use it globally, will investigate this.

Anyway I removed the unnecessary changes

@Rarst Rarst merged commit 620a161 into Rarst:master Mar 22, 2023
@Rarst
Copy link
Owner

Rarst commented Mar 22, 2023

Cheers!

FYI personally I have multiple versions of PHPUnit set up as PHARs in the system. I can have a lot of projects around, and sometimes within same IDE session, so zillion copies of PHPUnit can get hard on IDE and making sense of what's going on. More so if tests are supposed to work with multiple PHPUnit versions.

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