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

Jump to bottom

Require constructor property promotion #283

Merged
merged 2 commits into from Aug 26, 2022
Merged

Require constructor property promotion #283

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e0e1eed
Require constructor property promotion
greg0ire Jul 20, 2022
b7e1a4b
Update compatibility patch
greg0ire Jul 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/Doctrine/ruleset.xml
View file
Open in desktop
Expand Up @@ -144,6 +144,8 @@
<property name="enableMultipleSpacesBetweenModifiersCheck" value="true"/>
</properties>
</rule>
<!-- Require usage of constructor property promotion -->
<rule ref="SlevomatCodingStandard.Classes.RequireConstructorPropertyPromotion"/>
<!-- Forbid uses of multiple traits separated by comma -->
<rule ref="SlevomatCodingStandard.Classes.TraitUseDeclaration"/>
<!-- Require no spaces before trait use, between trait uses and one space after trait uses -->
Expand Down
6 changes: 3 additions & 3 deletions tests/expected_report.txt
View file
Open in desktop
Expand Up @@ -15,7 +15,7 @@ tests/input/ControlStructures.php 28 0
tests/input/doc-comment-spacing.php 11 0
tests/input/duplicate-assignment-variable.php 1 0
tests/input/EarlyReturn.php 7 0
tests/input/example-class.php 43 0
tests/input/example-class.php 44 0
tests/input/ExampleBackedEnum.php 3 0
tests/input/Exceptions.php 1 0
tests/input/forbidden-comments.php 14 0
Expand Down Expand Up @@ -50,9 +50,9 @@ tests/input/use-ordering.php 1 0
tests/input/useless-semicolon.php 2 0
tests/input/UselessConditions.php 21 0
----------------------------------------------------------------------
A TOTAL OF 428 ERRORS AND 0 WARNINGS WERE FOUND IN 46 FILES
A TOTAL OF 429 ERRORS AND 0 WARNINGS WERE FOUND IN 46 FILES
----------------------------------------------------------------------
PHPCBF CAN FIX 363 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
PHPCBF CAN FIX 364 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


12 changes: 1 addition & 11 deletions tests/fixed/example-class.php
View file
Open in desktop
Expand Up @@ -25,20 +25,10 @@ class Example implements IteratorAggregate
{
private const VERSION = PHP_VERSION - (PHP_MINOR_VERSION * 100) - PHP_PATCH_VERSION;

private int|null $foo = null;

/** @var string[] */
Copy link
Member Author

Choose a reason for hiding this comment

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

This comment is lost. Should I report a bug about it? I've tried adding an @param phpdoc to the constructor, and it stays.

In your projects, when you use CPP, do you use @param, or do you annotate properties, like this:

public function __construct(
    /** @var list<string> */
    private array $someProperty,
   …
) {

Copy link
Member

Choose a reason for hiding this comment

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

I use the following:

/** @param list<EntitySlot> $slots */
public function __construct(
    private readonly array $slots,
) {
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as @morozov

Copy link
Contributor

Choose a reason for hiding this comment

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

@greg0ire I can add some options to the sniff if needed (in August).

Copy link
Member

Choose a reason for hiding this comment

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

I think, this is fine. We have other sniffs plus static analysis in place that should already complain about the missing @param annotation.

Copy link
Member Author

@greg0ire greg0ire Jul 21, 2022 •
edited

Choose a reason for hiding this comment

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

There seems to be an agreement. What should I do then? Use @param in tests/fixed/example-class.php and fix the patches so that they apply cleanly? Or just keep as is so that this disappearance stays documented?

Copy link
Member

Choose a reason for hiding this comment

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

Or just keep as is so that this disappearance stays documented?

Keeping it as it is, is fine I guess.

private array $bar;

private bool $baz;

private ControlStructureSniff|int|string|null $baxBax = null;

public function __construct(int|null $foo = null, array $bar = [], bool $baz = false, $baxBax = 'unused')
public function __construct(private int|null $foo = null, private array $bar = [], private bool $baz = false, $baxBax = 'unused')
{
$this->foo = $foo;
$this->bar = $bar;
$this->baz = $baz;
$this->baxBax = $baxBax;
}

Expand Down