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

Code quality levels #5910

Merged
merged 4 commits into from
May 25, 2024
Merged

Conversation

carlos-granados
Copy link
Contributor

Add new withCodeQualityLevel() config function that allows you to add the quality level rules one at a time, similar to what you can do with the dead code rules and the type coverage rules. This avoids the problem of having too many problems to solve if you use codeQuality: true in withPreparedSets()

'mbstrrpos' => 'mb_strrpos',
'mbsubstr' => 'mb_substr',
]);
foreach (CodeQualityLevel::RULES_WITH_CONFIGURATION as $rectorClass => $configuration) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently there is only one rule with configuration but I made this future proof by allowing to have an array of them in the CodeQualityLevel class

]);
// the rule order matters, as its used in withCodeQualityLevel() method
// place the safest rules first, follow by more complex ones
$rectorConfig->rules(CodeQualityLevel::RULES);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use the list of rules defined in CodeQualityLevel instead of defining the list here


$this->rules = array_merge($this->rules, $levelRules);

foreach (CodeQualityLevel::RULES_WITH_CONFIGURATION as $rectorClass => $configuration) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To simplify level management I decided to always run the configured rules (currently there is only one) independently of the level chosen. If you would prefer to do something different, let me know

@TomasVotruba
Copy link
Member

Looks very good, thank you @carlos-granados 👍

@@ -33,6 +34,10 @@ public static function resolve(string $setFile): array
return TypeDeclarationLevel::RULES;
}

if (realpath($setFile) === realpath(SetList::CODE_QUALITY)) {
Copy link
Member

Choose a reason for hiding this comment

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

please craete variable for repetitive realpath($setFile) early

StaticToSelfStaticMethodCallOnFinalClassRector::class,
];

public const RULES_WITH_CONFIGURATION = [
Copy link
Member

@samsonasik samsonasik May 24, 2024

Choose a reason for hiding this comment

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

add @var doc:

Suggested change
public const RULES_WITH_CONFIGURATION = [
/**
* @var array<class-string<RectorInterface>, mixed[]>
*/
public const RULES_WITH_CONFIGURATION = [

@carlos-granados carlos-granados deleted the code-quality-levels branch May 25, 2024 06:09
@carlos-granados carlos-granados restored the code-quality-levels branch May 25, 2024 06:11
@carlos-granados
Copy link
Contributor Author

I accidentally closed the PR, re-opened

@carlos-granados
Copy link
Contributor Author

@samsonasik I added the changes you suggested

@samsonasik
Copy link
Member

Rebase seems needed to make CI green

@carlos-granados
Copy link
Contributor Author

Rebased. When I did the rebase there were a couple of ECS failures due to the order of some imports. Even though this was unrelated to my PR, I have included the fixes here. If that is not OK, let me know and I will remove them

Copy link
Member

@samsonasik samsonasik left a comment

Choose a reason for hiding this comment

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

Let's give it a try

@samsonasik samsonasik merged commit 650ae6a into rectorphp:main May 25, 2024
40 checks passed
@samsonasik
Copy link
Member

Thank you @carlos-granados

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