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

Feat: php-cs-fixer #154

Merged
merged 8 commits into from
Sep 9, 2024
Merged

Feat: php-cs-fixer #154

merged 8 commits into from
Sep 9, 2024

Conversation

Rom1-B
Copy link
Contributor

@Rom1-B Rom1-B commented Sep 5, 2024

Add php-cs-fixer

.php-cs-fixer.php Outdated Show resolved Hide resolved
Comment on lines 55 to 77
$optionalRules = [
'array_indentation' => true,
'array_syntax' => ['syntax' => 'short'],
'binary_operator_spaces' => ['default' => 'align_single_space_minimal'],
'blank_line_after_namespace' => true,
'blank_line_after_opening_tag' => true,
'blank_line_before_statement' => ['statements' => ['return']],
'braces' => ['allow_single_line_closure' => true],
'cast_spaces' => ['space' => 'single'],
'class_attributes_separation' => ['elements' => ['method' => 'one']],
'concat_space' => ['spacing' => 'one'],
'declare_equal_normalize' => ['space' => 'single'],
'function_typehint_space' => true,
'lowercase_cast' => true,
'no_whitespace_in_blank_line' => true,
'single_blank_line_at_eof' => true,
'single_quote' => true,
'space_after_semicolon' => true,
'trailing_comma_in_multiline' => ['elements' => ['arrays']],
'trim_array_spaces' => true,
'unary_operator_spaces' => true,
'whitespace_after_comma_in_array' => true,
];
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not against some strict rules, but I guess that almost every PR will require an additional commit to fix CS violations.

Did you talk about it with the other developers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but that's also the goal of this PR. Since NEWS doesn't have any other PRs in progress, it's being used as a test case ;)

Comment on lines 6 to 35
/**
* Read excluded paths from .gitignore
*
* @param string $dir
*
* @return string[]
*/
function getGitignorePaths(string $dir): array
{
$gitignoreFile = $dir . '/.gitignore';
$paths = [];

if (!file_exists($gitignoreFile)) {
return $paths;
}

$lines = file($gitignoreFile, FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES);
if (!empty($lines)) {
foreach ($lines as $line) {
// Ignore comments and empty lines
if (strpos($line, '#') === 0 || trim($line) === '') {
continue;
}
// Add relative paths
$paths[] = trim($line);
}
}

return $paths;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the documentation:

Default finder ignores DIR . "/vendor" dir, “hidden” paths (ones starting with a dot) and VCS paths (e.g. .git), and filter only for *.php files.

What kind of files do you want to exclude with this function ? Most of the time, the defaut exclusions will do the job, and manually adding a specific dir is probably simplier than maintaining this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially intended to create a generic configuration that could be identical across each plugin, allowing for simple copy-and-paste. However, it might not be optimal, so I will remove it.

@Rom1-B
Copy link
Contributor Author

Rom1-B commented Sep 6, 2024

Can I remove squizlabs/php_codesniffer ?

@AdrienClairembault
Copy link
Contributor

Can I remove squizlabs/php_codesniffer ?

Sure.

Copy link
Contributor

@cedric-anne cedric-anne left a comment

Choose a reason for hiding this comment

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

Some changes seems related to usage of stricter rules in the first commits of the PR, but keeping them should not be a problem.


$rules = [
'@PER-CS2.0' => true,
'trailing_comma_in_multiline' => ['elements' => ['arguments', 'array_destructuring', 'arrays']], // For PHP 7.4 compatibility
Copy link
Contributor

Choose a reason for hiding this comment

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

The PER 2.0 is an extension of the PER 1.0 (the new name of the PSR 12) and its specifications are not compatible on some points with PHP 7.x.

There are 2 solutions for this:

  1. Use the PSR 12 right now and move to the PER 2.0 later.
  2. Use the PER 2.0 right now with some overriden rules, and remove these overrides later.

Both are OK to me, but I guess that the current solution will be less subject to conflicts and is therefore preferable.

@Rom1-B Rom1-B merged commit 2a8df0a into pluginsGLPI:main Sep 9, 2024
3 checks passed
@Rom1-B Rom1-B deleted the feat_php-cs-fixer branch September 9, 2024 06:28
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.

4 participants