-
-
Notifications
You must be signed in to change notification settings - Fork 280
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
Make the cyclomatic complexity limit configurable. #190
Make the cyclomatic complexity limit configurable. #190
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's perfect, I would just rename limit
to maxComplexity
. What do you think?
Good idea, I have changed it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tvbeek Can you run composer test
locally? Tests are not passing.
Yes, sorry I just saw it. Currently working on it. |
… errors from phpinsights
It passed the test, do you have anything else that you like to have changed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for the contribution!
If it's not too much trouble could you add a test for it? :)
Great addition 👍 Thank you @tvbeek 🙏 If you can, can you also update documentation to help users to know how to configure it ? You just have to add in https://github.com/nunomaduro/phpinsights/blob/master/docs/insights/complexity.md something like + <details>
+ <summary>Configuration</summary>
+
+ ```php
+ \NunoMaduro\PhpInsights\Domain\Insights\CyclomaticComplexityIsHigh::class => [
+ 'maxComplexity' => 5
+ ]
+ ```
+ </details> after Insight Class: |
@olivernybroe There wasn't a test for this insight but I will create it. @Jibbarth thanks for pointing out to the documentation. I have added it. |
I have added a test for the CyclomaticComplexityIsHigh Insight that test this config (and the normal behavior) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work!
This PR has everything.
Tests, docs and a nice contribution 🎉
@nunomaduro @Jibbarth I'll say this is ready for merge. So if one of you agree with me, just merge it in :) |
…n and update the documentation so that the configuration is visible. Thanks @Jibbarth for the feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 Perfect
@Jibbarth @olivernybroe Tonight I will release a new version of php insights. |
Thanks @tvbeek for this pull request. |
I found that I wanted to change the limit of the cyclomatic complexity insight. Currently this is hard coded fixed on 5. So with this PR I add the option to change it.