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

Fix annotation groups usage #91

Merged
merged 1 commit into from
Nov 1, 2018
Merged

Fix annotation groups usage #91

merged 1 commit into from
Nov 1, 2018

Conversation

lcobucci
Copy link
Member

The main issue here is that both PHPDocumentor and PHPUnit relies on @uses annotation - with different meaning.

I personally wouldn't use @uses the way PHPDocumentor defines but I wanted to first have the build fixed before suggesting changes.

BTW this would be my suggestion for annotation groups:

https://github.com/lcobucci/coding-standard/blob/940d248df0a3ff0d3ccfa3ea44c8566313863ce2/src/Lcobucci/ruleset.xml#L33-L52

Applying the fixes to the configuration.
@lcobucci lcobucci self-assigned this Oct 14, 2018
@lcobucci lcobucci removed their assignment Oct 14, 2018
@lcobucci lcobucci mentioned this pull request Oct 15, 2018
@Ocramius
Copy link
Member

@lcobucci I don't understand the diff: you just moved @uses up?

@lcobucci
Copy link
Member Author

@Ocramius yeap... the input has @uses in the incorrect place (according to our config). This PR fixes it, thus making the build pass again.

As I tried to explain, our configuration groups @uses with @see and @link (which is the meaning that PHP Documentor gives for that annotation):

<rule ref="SlevomatCodingStandard.Commenting.DocCommentSpacing">
    <properties>
        <property name="linesCountBeforeFirstContent" value="0"/>
        <property name="linesCountAfterLastContent" value="0"/>
        <property name="linesCountBetweenDescriptionAndAnnotations" value="1"/>
        <property name="linesCountBetweenAnnotationsGroups" value="1"/>
        <property name="annotationsGroups" type="array">
            <element value="@internal, @deprecated,"/>
            <element value="@link, @see, @uses,"/>
            <element value="@ORM\, @ODM\,"/>
            <element value="@param"/>
            <element value="@return"/>
            <element value="@throws"/>
        </property>
    </properties>
</rule>

And our files were using PHPUnit's meaning of @uses (explicit coverage tracking).

I thought that we shouldn't discuss modifying the meaning of that annotation for our CS without having the build stable first.

@alcaeus
Copy link
Member

alcaeus commented Oct 15, 2018

Isn't there a @covers annotation for PHPUnit? What's the difference between that and @uses? If we can avoid the double meaning, I'm all for doing so.

@Ocramius
Copy link
Member

@covers indicates coverage, and allows setting up the test suite in such a way that any coverage outside @covers leads to a Risky test, and therefore to a non-zero exit code.

@uses is used to indicate code that is used in a certain test, but is more like "other, unrelated units".

The typical example is a test like following:

/**
 * @covers \My\Authentication
 * @uses \My\UserId
 */
final class TestAuthentication extends TestCase
{
    // ...
}

@lcobucci
Copy link
Member Author

lcobucci commented Oct 15, 2018

@Ocramius I know, I think I've failed to explain again. Our groups were set based on this: https://docs.phpdoc.org/references/phpdoc/tags/uses.html and not on PHPUnit's meaning of @uses.

My intention with this PR was not to discuss whether this is right or wrong, our build is broken and this PR fixes it.

Please be explicit if you'd like to have the configuration changed, I can change the PR.

@Ocramius
Copy link
Member

I don't mind either way. I don't think @uses is to be used like in PHPDocumentor, as it is exposing implementation details in the documentation of an API, so we should probably phase it out long-term (not sure how to do that properly in this repo)

@lcobucci
Copy link
Member Author

Let's ask the person who configured the groups then... @Majkl578 what's your take here, mate?

@SenseException
Copy link
Member

My intention with this PR was not to discuss whether this is right or wrong, our build is broken and this PR fixes it.

I think a fixed master build would be nice. It makes it easier for contributors to keep focus on their own tests. @lcobucci Are you going to add your group-suggestion in another PR or are you going to add it here after feedback?

@lcobucci
Copy link
Member Author

@lcobucci Are you going to add your group-suggestion in another PR or are you going to add it here after feedback?

I can do either, my initial thought was to do the former.

@alcaeus
Copy link
Member

alcaeus commented Nov 1, 2018

🚢

@alcaeus alcaeus merged commit de5da83 into master Nov 1, 2018
@alcaeus alcaeus deleted the fix-annotation-groups branch November 1, 2018 20:26
@lcobucci lcobucci added this to the 5.0.1 milestone Jan 31, 2019
@lcobucci lcobucci added bug and removed Improvement labels Jan 31, 2019
@lcobucci lcobucci changed the title Add annotation group violations to expected report Fix annotation groups usage Jan 31, 2019
@lcobucci lcobucci mentioned this pull request Jan 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants