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

Make all annotations useful by default #108

Conversation

Majkl578
Copy link
Contributor

@Majkl578 Majkl578 commented Feb 6, 2019

Slevomat CS 5.0 drops usefulAnnotations so we are forced to switch to allAnnotationsAreUseful instead.
From now on there is no whitelist for useful annotations, only the blacklist provided by ForbiddenAnnotations.

Sadly I'm still not convinced this is a good step forward though.

@Majkl578 Majkl578 added this to the 6.0.0 milestone Feb 6, 2019
@Majkl578 Majkl578 requested a review from a team as a code owner February 6, 2019 23:05
@Majkl578 Majkl578 force-pushed the TypeHintDeclaration-allAnnotationsAreUseful branch from e0c7c47 to a95e66c Compare February 6, 2019 23:06
@Majkl578 Majkl578 changed the title Enable TypeHintDeclaration.allAnnotationsAreUseful Make all annotations useful by default Feb 6, 2019
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

🤷‍♂️

Only option I see is creating a new sniff maintained by us that keeps old behaviour. Other than that, nothing we can do.

Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

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

@Majkl578 could you provide a reference to the discussion of the deprecation?

Copy link
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

I honestly fail to see the value of maintaining a whitelist and a blacklist, do think this is a good thing.

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

@ORM\
This one is more about a fix alias for Doctrine annotations, which is used by the documentation, but excludes other alias choices for the same annotations. Technically, the annotations of Doctrine were excluded.

@lcobucci lcobucci merged commit e8f939b into doctrine:master Feb 7, 2019
@lcobucci lcobucci self-assigned this Feb 7, 2019
@Majkl578 Majkl578 deleted the TypeHintDeclaration-allAnnotationsAreUseful branch February 7, 2019 13:51
@Majkl578
Copy link
Contributor Author

Majkl578 commented Feb 7, 2019

@morozov I don't think there was discussion about this - it was deprecated at the moment allAnnotationsAreUseful was added: slevomat/coding-standard@85ca537. We'd need to ask @kukulich himself.

@kukulich
Copy link
Contributor

kukulich commented Feb 7, 2019

There was discussion among all Slevomat CS developers so between me and nobody else :D It's better to have special (and smaller) sniffs to solve special tasks. ForbiddenAnnotationsSniff is better. There's always possibility to implemented new whitelist option for the ForbiddenAnnotationsSniff

@carusogabriel
Copy link
Contributor

Why didn't we update to Slevomat/CS v5.0 first?

@Majkl578
Copy link
Contributor Author

Majkl578 commented Feb 7, 2019

@carusogabriel Because this was pre-requisite for the upgrade.

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

7 participants