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.

Sign up for GitHub

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: Remove the most risky fixers from the next version #98

Merged
merged 1 commit into from
May 14, 2024
Merged

Feat: Remove the most risky fixers from the next version #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 0 additions & 32 deletions ecs.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,6 @@
use PhpCsFixer\Fixer\FunctionNotation\LambdaNotUsedImportFixer;
use PhpCsFixer\Fixer\FunctionNotation\NoUnreachableDefaultArgumentValueFixer;
use PhpCsFixer\Fixer\FunctionNotation\NoUselessSprintfFixer;
use PhpCsFixer\Fixer\FunctionNotation\PhpdocToParamTypeFixer;
use PhpCsFixer\Fixer\FunctionNotation\PhpdocToPropertyTypeFixer;
use PhpCsFixer\Fixer\FunctionNotation\PhpdocToReturnTypeFixer;
use PhpCsFixer\Fixer\FunctionNotation\ReturnTypeDeclarationFixer;
use PhpCsFixer\Fixer\FunctionNotation\VoidReturnFixer;
use PhpCsFixer\Fixer\Import\NoLeadingImportSlashFixer;
Expand Down Expand Up @@ -133,14 +130,10 @@
use PhpCsFixer\Fixer\Whitespace\NoExtraBlankLinesFixer;
use PhpCsFixer\Fixer\Whitespace\NoWhitespaceInBlankLineFixer;
use PhpCsFixer\Fixer\Whitespace\TypeDeclarationSpacesFixer;
use SlevomatCodingStandard\Sniffs\Classes\RequireConstructorPropertyPromotionSniff;
use SlevomatCodingStandard\Sniffs\ControlStructures\RequireNullSafeObjectOperatorSniff;
use SlevomatCodingStandard\Sniffs\Exceptions\ReferenceThrowableOnlySniff;
use SlevomatCodingStandard\Sniffs\Functions\RequireTrailingCommaInCallSniff;
use SlevomatCodingStandard\Sniffs\Functions\RequireTrailingCommaInDeclarationSniff;
use SlevomatCodingStandard\Sniffs\TypeHints\ParameterTypeHintSniff;
use SlevomatCodingStandard\Sniffs\TypeHints\PropertyTypeHintSniff;
use SlevomatCodingStandard\Sniffs\TypeHints\ReturnTypeHintSniff;
use SlevomatCodingStandard\Sniffs\TypeHints\UnionTypeHintFormatSniff;
use Symplify\CodingStandard\Fixer\Commenting\ParamReturnAndVarTagMalformsFixer;
use Symplify\EasyCodingStandard\Config\ECSConfig;
Expand Down Expand Up @@ -377,26 +370,13 @@
ReferenceThrowableOnlySniff::class,
// The @param, @return, @var and inline @var annotations should keep standard format
ParamReturnAndVarTagMalformsFixer::class,
// Takes `@var` annotation of non-mixed types and adjusts accordingly the property signature.
PhpdocToPropertyTypeFixer::class,
// Takes `@param` annotations of non-mixed types and adjusts accordingly the function signature.
PhpdocToParamTypeFixer::class,
// Takes `@return` annotation of non-mixed types and adjusts accordingly the function signature.
PhpdocToReturnTypeFixer::class,
// Promote constructor properties
// @see https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/5956
RequireConstructorPropertyPromotionSniff::class,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we want this? We are already using it everywhere where we have PHP 8 🤔

Copy link
Contributor Author

@OndraM OndraM May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

See PR description - we want this, but I think for some teams may be preferred to have the ability to easily split the upgrade into two phases:

  1. Upgrade config format but with no much required code style changes. For this I will release version 4.0.
  2. Upgrade code to comply to all new shiny fixers. For this I will release version 4.1, immediately after 4.0.

The most agile teams will upgrade directly to 4.1 and get all those fixers. The conservative teams or teams with bigger codebase will have option to upgrade to 4.0 first, and once they are cleared, they can continue with upgrade to 4.1, which may be more difficult, because the version will contain tens of new fixers, see #95 .

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry, missed the description. Yea, it makes sense 👍

Copy link
Contributor

@D0L1K D0L1K May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But wait, I've just realised - those checks are already present in 3.3.x under ecs-8.0.php. So teams using PHP 8.0+ with those rules are already using them and if we remove them, it will create only more mess in the codebase. And if not, they can start with using them as step 1 and afterwards just switch to 4.0.0 and edit configs as step 2.
I just don't think removing of rules which are already used in some projects is a good idea tbh

Copy link
Contributor Author

@OndraM OndraM May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we remove these checks, nothing will happen. The rules will just not be checked, but no change in code will be triggered. So I don't see a risk of creating any mess in the codebase.

Also, if you already use the 8.0 checks, you can simply upgrade to 4.1, right?

These rules were only "optional", they were not part of the default setup, because you have to manually require the php-8.0.php file on top of the ecs.php. Because of this, I see them as "newly added default checks", because they are now "forced". So I am trying to be a bit conservative in this... Also those checks are risky, they can change the code behavior, so I want people to be really cautions when they will be adding them.

Copy link
Contributor

@D0L1K D0L1K May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yea I know, what I've meant by that is that the code, which is not valid in 3.3 with 8.0 rules used and which also will not be valid in 4.1, will be valid in 4.0. So that can create a situation where some code will be formated by those rules from the previous version and some not when 4.0 is used, which will result in more fixes with 4.1.
However, as far as 4.1 will be released with 4.0 at the same time I'm ok with it :)


// switch -> match
// @see https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/5894

// Require \Stringable interface in classes implementing __toString() method
// > it may probably be a phpstan rule, more than cs rule - since it needs a class hierarchy to solve this
// @see https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/6235
PropertyTypeHintSniff::class,

ParameterTypeHintSniff::class,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing those two?

Copy link
Contributor

Choose a reason for hiding this comment

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

And if we rly don't want them, maybe we should also remove https://github.com/lmc-eu/php-coding-standard/blob/af892d76815b3197529bec671ffe4e1df5f5bf59/ecs.php#L521?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, they should be removed from this as well.

ReturnTypeHintSniff::class,

// Multi-line arguments list in function/method declaration must have a trailing comma
RequireTrailingCommaInDeclarationSniff::class,
Expand Down Expand Up @@ -531,18 +511,6 @@
// Allow single line closures
ScopeClosingBraceSniff::class . '.ContentBefore' => null,

// Skip unwanted rules from PropertyTypeHintSniff
PropertyTypeHintSniff::class . '.' . PropertyTypeHintSniff::CODE_MISSING_TRAVERSABLE_TYPE_HINT_SPECIFICATION => null,
PropertyTypeHintSniff::class . '.' . PropertyTypeHintSniff::CODE_MISSING_ANY_TYPE_HINT => null,

// Skip unwanted rules from ParameterTypeHintSniff
ParameterTypeHintSniff::class . '.' . ParameterTypeHintSniff::CODE_MISSING_TRAVERSABLE_TYPE_HINT_SPECIFICATION => null,
ParameterTypeHintSniff::class . '.' . ParameterTypeHintSniff::CODE_MISSING_ANY_TYPE_HINT => null,

// Skip unwanted rules from ReturnTypeHintSniff
ReturnTypeHintSniff::class . '.' . ReturnTypeHintSniff::CODE_MISSING_TRAVERSABLE_TYPE_HINT_SPECIFICATION => null,
ReturnTypeHintSniff::class . '.' . ReturnTypeHintSniff::CODE_MISSING_ANY_TYPE_HINT => null,

// We use declare(strict_types=1); after opening tag
BlankLineAfterOpeningTagFixer::class => null,
]);
5 changes: 4 additions & 1 deletion tests/Integration/Fixtures/NewPhpFeatures.correct.php.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@ namespace Lmc\CodingStandard\Integration\Fixtures;

class NewPhpFeatures
{
public function __construct(private string $someString) // RequireConstructorPropertyPromotionSniff
private string $someString;

public function __construct(string $someString)
{
$this->someString = $someString;
}

public function php80features(
Expand Down
2 changes: 1 addition & 1 deletion tests/Integration/Fixtures/NewPhpFeatures.wrong.php.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class NewPhpFeatures
{
private string $someString;

public function __construct(string $someString) // RequireConstructorPropertyPromotionSniff
public function __construct(string $someString)
{
$this->someString = $someString;
}
Expand Down