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

Crumble removed TypeHintDeclaration sniff #139

Closed
wants to merge 5 commits into from
Closed

Crumble removed TypeHintDeclaration sniff #139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
efa7ae9
Require slevomat/coding-standard v6
simPod Dec 5, 2019
27d7fd4
Crumble removed TypeHintDeclaration sniff
simPod Dec 3, 2019
a52dae5
test traversableTypeHints
simPod Dec 5, 2019
67ca9e1
add more traversable types
simPod Dec 5, 2019
8d8df28
cant fix, sure
simPod Dec 5, 2019
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
4 changes: 2 additions & 2 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@
"require": {
"php": "^7.2",
"dealerdirect/phpcodesniffer-composer-installer": "^0.5.0",
"slevomat/coding-standard": "dev-master#469368cc6d8fe83aceba259ef65f1c0a2f63055b",
"squizlabs/php_codesniffer": "^3.5.0"
simPod marked this conversation as resolved.
Show resolved Hide resolved
"slevomat/coding-standard": "^6.0",
"squizlabs/php_codesniffer": "^3.5"
},
"config": {
"sort-packages": true
Expand Down
41 changes: 38 additions & 3 deletions lib/Doctrine/ruleset.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,17 +334,52 @@
<!-- Require types to be written as natively if possible;
require iterable types to specify phpDoc with their content;
forbid useless/duplicated information in phpDoc -->
<rule ref="SlevomatCodingStandard.TypeHints.TypeHintDeclaration">
<rule ref="SlevomatCodingStandard.TypeHints.ParameterTypeHint">
<properties>
<property name="allAnnotationsAreUseful" value="true"/>
<property name="enableEachParameterAndReturnInspection" value="true"/>
<property name="traversableTypeHints" type="array">
<element value="Traversable"/>
<element value="Iterator"/>
<element value="IteratorAggregate"/>
<element value="Doctrine\Common\Collections\Collection"/>
Copy link
Member

Choose a reason for hiding this comment

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

It isn't fully clear to me why we are targeting Collection as a specific type here.

Would need:

  1. an example (before/after test)
  2. to know how this copes with @var Collection<TKey, TVar>

Copy link
Member

Choose a reason for hiding this comment

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

It isn't fully clear to me why we are targeting Collection as a specific type here.

Same reason why all other sniffs of this kind add this: to ensure that a parameter with a Collection type hint also gets a proper @param or @return annotation that clarifies its type (as is done with array).

+1 on adding tests for this.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but shouldn't this be done with any Traversable then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was migrated from deprecated sniff.

According to docs, this forces to write \Doctrine\Common\Collections\Collection|Foo[].

Personally, I always write \Doctrine\Common\Collections\Collection<Foo>. If it was on me, I'd throw it away.

Copy link
Member

Choose a reason for hiding this comment

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

array and iterable are always treated as such. This list is in addition to the ones where the sniff requires it by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alcaeus by default it seems to be empty https://github.com/slevomat/coding-standard/blob/7a9530a05f7b93cd3cdb8c475f743f97a447aae4/SlevomatCodingStandard/Sniffs/TypeHints/ParameterTypeHintSniff.php#L343-L351

Copy link
Contributor

Choose a reason for hiding this comment

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

@alcaeus Only iterable and array are covered by default. You can use DisallowArrayTypeHintSyntaxSniff and use always generic.

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 really like the idea of enabling DisallowArrayTypeHintSyntaxSniff

WDY-all-T?

Copy link
Member

Choose a reason for hiding this comment

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

Please create a separate PR for that so the deciders can vote on it. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I add only Traversable there it is not applied to eg. Iterator. So we have to explicitly name everything, that implements Traversable

</property>
</properties>
</rule>
<rule ref="SlevomatCodingStandard.TypeHints.PropertyTypeHint">
<properties>
<property name="enableNativeTypeHint" value="false"/>
<property name="traversableTypeHints" type="array">
<element value="Traversable"/>
<element value="Iterator"/>
<element value="IteratorAggregate"/>
<element value="Doctrine\Common\Collections\Collection"/>
</property>
</properties>
</rule>
<rule ref="SlevomatCodingStandard.TypeHints.ReturnTypeHint">
<properties>
<property name="traversableTypeHints" type="array">
<element value="Traversable"/>
<element value="Iterator"/>
<element value="IteratorAggregate"/>
<element value="Doctrine\Common\Collections\Collection"/>
</property>
</properties>
</rule>
<!-- Forbid useless @var for constants -->
<rule ref="SlevomatCodingStandard.TypeHints.UselessConstantTypeHint"/>
<!-- Forbid useless phpDocs for functions -->
<rule ref="SlevomatCodingStandard.Commenting.UselessFunctionDocComment">
<properties>
<property name="traversableTypeHints" type="array">
<element value="Traversable"/>
<element value="Iterator"/>
<element value="IteratorAggregate"/>
<element value="Doctrine\Common\Collections\Collection"/>
</property>
</properties>
</rule>
<!-- Forbid useless inherit doc comment -->
<rule ref="SlevomatCodingStandard.Commenting.UselessInheritDocComment"/>
<!-- Forbid duplicated variables assignments -->
<rule ref="SlevomatCodingStandard.Variables.DuplicateAssignmentToVariable"/>
<!-- Forbid useless variables -->
Expand Down
3 changes: 2 additions & 1 deletion tests/expected_report.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,13 @@ tests/input/superfluous-naming.php 11 0
tests/input/test-case.php 8 0
tests/input/trailing_comma_on_array.php 1 0
tests/input/traits-uses.php 11 0
tests/input/type-hints.php 4 0
tests/input/UnusedVariables.php 1 0
tests/input/use-ordering.php 1 0
tests/input/useless-semicolon.php 2 0
tests/input/UselessConditions.php 20 0
----------------------------------------------------------------------
A TOTAL OF 289 ERRORS AND 0 WARNINGS WERE FOUND IN 33 FILES
A TOTAL OF 293 ERRORS AND 0 WARNINGS WERE FOUND IN 34 FILES
----------------------------------------------------------------------
PHPCBF CAN FIX 232 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------
Expand Down
24 changes: 24 additions & 0 deletions tests/fixed/type-hints.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php

declare(strict_types=1);

namespace TypeHints;

use Iterator;
use Traversable;

class TraversableTypeHints
{
/** @var Traversable */
private $parameter;

/**
* @param Iterator $iterator
*
* @return Traversable
*/
public function get(Iterator $iterator) : Traversable
{
return $this->parameter;
}
}
24 changes: 24 additions & 0 deletions tests/input/type-hints.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php

declare(strict_types=1);

namespace TypeHints;

use Iterator;
use Traversable;

class TraversableTypeHints
{
/** @var Traversable */
private $parameter;

/**
* @param Iterator $iterator
*
* @return Traversable
*/
public function get(Iterator $iterator) : Traversable
{
return $this->parameter;
}
}