Skip to content

Code quality levels#5910

Merged
samsonasik merged 4 commits intorectorphp:mainfrom
carlos-granados:code-quality-levels
May 25, 2024
Merged

Code quality levels#5910
samsonasik merged 4 commits intorectorphp:mainfrom
carlos-granados:code-quality-levels

Conversation

@carlos-granados
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Looks very good, thank you @carlos-granados 👍

@TomasVotruba TomasVotruba requested a review from samsonasik May 24, 2024 14:13
return TypeDeclarationLevel::RULES;
}

if (realpath($setFile) === realpath(SetList::CODE_QUALITY)) {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

I accidentally closed the PR, re-opened

@carlos-granados
Copy link
Copy Markdown
Contributor Author

@samsonasik I added the changes you suggested

@samsonasik
Copy link
Copy Markdown
Member

Rebase seems needed to make CI green

@carlos-granados
Copy link
Copy Markdown
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
Copy Markdown
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
@samsonasik
Copy link
Copy Markdown
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