Skip to content

[Proposal] Remove unstable Annotation sniffs and replace with OOB ones #39

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

Closed
lenaorobei opened this issue Feb 12, 2019 · 2 comments · Fixed by #60
Closed

[Proposal] Remove unstable Annotation sniffs and replace with OOB ones #39

lenaorobei opened this issue Feb 12, 2019 · 2 comments · Fixed by #60
Assignees
Labels
accepted New rule is accepted event: MageTestFest2019 MageTestFest contributions Progress: good first issue Issues is easy to get started with proposal New rule proposal

Comments

@lenaorobei
Copy link
Contributor

lenaorobei commented Feb 12, 2019

Reason

Rules under Magento/Annotation folder rely on token index instead of token content and do not verify findings (findPrevious and findNext can return false). This leads to false-positive findings and and errors during execution (#38).

Solution

  1. Remove unstable Magento/Annotation sniffs.
  2. Add out-of-box PHP CodeSniffer rules that cover the bigger part of functionality + add extra useful and efficient checks.
    <!--Ensures doc blocks follow basic formatting.-->
    <rule ref="Generic.Commenting.DocComment">
        <severity>5</severity>
        <type>warning</type>
    </rule>
    
    <!--Parses and verifies the class doc comment.
        Verifies that:
      - A class DocBlock comment exists.
      - The comment uses the correct DocBlock style.
      - There are no blank lines after the class comment.
      - No tags are used in the DocBlock.-->
    <rule ref="Squiz.Commenting.ClassComment">
        <severity>5</severity>
        <type>warning</type>
    </rule>
    
    <!--Allow tags in the aforesaid sniff.-->
    <rule ref="Squiz.Commenting.ClassComment.TagNotAllowed">
        <severity>0</severity>
        <type>warning</type>
    </rule>

    <!--Parses and verifies the doc comments for functions.-->
    <rule ref="Squiz.Commenting.FunctionComment">
        <severity>5</severity>
        <type>warning</type>
    </rule>

    <!--Verifies that a @throws tag exists for each exception type a function throws.-->
    <rule ref="Squiz.Commenting.FunctionCommentThrowTag">
        <severity>5</severity>
        <type>warning</type>
    </rule>

    <!--Parses and verifies the variable doc comment.-->
    <rule ref="Squiz.Commenting.VariableComment">
        <severity>5</severity>
        <type>warning</type>
    </rule>
  1. Implement sniffs for missing rule: @inheritdoc formatting - issue [New Rule] Implement sniff for functions PHPDoc formatting #54

Bonus

Some of the OOB sniffs detect fixable issues that can be automatically fixed by phpcbf tool.

@lenaorobei lenaorobei added proposal New rule proposal need to discuss Rule requires discussion labels Feb 12, 2019
@lenaorobei
Copy link
Contributor Author

@orlangur I believe you have some thought to say here 🙂

@paliarush
Copy link
Contributor

Let's do the following:

  1. Leave annotation sniffs in the Magento repository only. Remove all other sniffs from the Magento repository.
  2. Ruleset in Magento repository should reference the new unified ruleset. Additionally it should still enable existing annotations sniff and disable standard annotation sniffs defined in the unified ruleset.
  3. After initial release, non-standard annotation checks should stay in custom sniff and it should be moved to the new unified repository and included into unified ruleset. As a result there will be no custom rules defined in the Magento repository and all custom ones will be defined in the unified repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted New rule is accepted event: MageTestFest2019 MageTestFest contributions Progress: good first issue Issues is easy to get started with proposal New rule proposal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants