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

Crumble removed TypeHintDeclaration sniff #139

Closed
wants to merge 5 commits into from
Closed

Crumble removed TypeHintDeclaration sniff #139

wants to merge 5 commits into from

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Dec 4, 2019

Introduced in slevomat/coding-standard@7db89be

Disable TypeHintDeclaration
Enable

  • ParameterTypeHint
  • PropertyTypeHint
  • ReturnTypeHint
  • UselessFunctionDocComment

Build currently fails, there are some features on by default for PHP 7.4. Have to investigate what's the cause and if I can fix it yet.

@simPod simPod requested a review from a team as a code owner December 4, 2019 01:13
@kukulich
Copy link
Contributor

kukulich commented Dec 4, 2019

@simPod You should set option “enableNativeTypeHint” for PropertyTypeHint to false

@simPod
Copy link
Contributor Author

simPod commented Dec 4, 2019

@kukulich did the trick, thanks!

composer.json Outdated Show resolved Hide resolved
alcaeus
alcaeus previously approved these changes Dec 4, 2019
@alcaeus alcaeus added this to the 7.0.0 milestone Dec 4, 2019
composer.json Show resolved Hide resolved
alcaeus
alcaeus previously approved these changes Dec 4, 2019
@alcaeus alcaeus requested a review from a team December 4, 2019 12:17
carusogabriel
carusogabriel previously approved these changes Dec 4, 2019
<rule ref="SlevomatCodingStandard.TypeHints.ParameterTypeHint">
<properties>
<property name="traversableTypeHints" type="array">
<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.

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

Disable TypeHintDeclaration
Enable
- ParameterTypeHint
- PropertyTypeHint
- ReturnTypeHint
- UselessFunctionDocComment
@simPod simPod dismissed stale reviews from carusogabriel and alcaeus via 27d7fd4 December 5, 2019 09:39
@simPod
Copy link
Contributor Author

simPod commented Dec 5, 2019

Superseded by #144

@simPod simPod closed this Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants