@sgiehl opened this Pull Request on December 17th 2021 Member

Description:

In order to get our code base a bit cleaner. I'd like to introduce some additional coding style checks.
We already have a GitHub action that will automatically add comments to pull requests if any check fails.

This PR adds the additional php cs rule

    <!-- Forbid unused use statements -->
    <rule ref="SlevomatCodingStandard.Namespaces.UnusedUses">
        <properties>
            <property name="searchAnnotations" value="true"/>
            <property name="ignoredAnnotations" type="array">
                <element value="<a class='mention' href='https://github.com/group'>@group</a>"/>
                <element value="<a class='mention' href='https://github.com/api'>@api</a>"/>
            </property>
        </properties>
    </rule>

I was already trying to remove those unused use statements in my PRs and also suggested to remove them in any review where I spotted some. To make that a bit simpler I guess it's better to simply "enforce" that.

Note: Unused use statements can in most cases be auto fixed, so simply running a ./vendor/bin/phpcbf will remove them anywhere. Did that for this PR as well, so they are already cleaned up.

Review

@tsteur commented on December 19th 2021 Member

@sgiehl what are the benefits of not having unused use statements? or what are the downsides when they are in there?

Be great to know, I can't really see downsides so far. Just wanting to avoid in the end that we have to remove unused use statements just to get a build to pass and adding few extra steps and making getting things merged take longer etc for maybe not much of a benefit in the end.

@sgiehl commented on December 19th 2021 Member

Having unused use statements hurts the same as having unused code. There's no direct benefit in removing them (besides the bytes it saves), but having messed up code will make the code more and more unmaintainable and unreadable.
This one is actually more a start to introduce some more machanisms to improve our code quality.
In the device detector repo I already have a lot more rules active (see https://github.com/matomo-org/device-detector/blob/master/phpcs.xml), also including some coding style rules. I would love to introduce most of such rules here as well over time, but some rules we should discuss in the team for sure.
Also as most of such rules are autofixable using phpcbf it's actually not really more work. phpcs/phpcbf can even be integrated in PHPStorm and other IDEs.

@tsteur commented on December 19th 2021 Member

Maybe to prevent it messing up over time we could have either

  • a process say every 6 months and clean them up all at once (we more efficient then having it to do every single time)
  • or eventually have it removed automatically but added commits might also cause issues in daily workflow as you then need to pull again changes committed from github etc.

Not sure how messed up it otherwise is. Most of the time the IDE would probably hide them anyway and they probably don't interrupt looking at the code etc since they are at the beginning. Maybe like a 6 monthly process to run the code to remove them automatically be good enough compared to spending time quite often to remove them every time etc?

@sgiehl commented on December 19th 2021 Member

If the IDE is configured properly that should be done automatically. Otherwise it's not a real big deal to run a command before doing a commit (or once if the build failed). And as mentioned it's actually not about the unused imports only. There is much more we could and should do about code quality

@github-actions[bot] commented on December 27th 2021 Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

Powered by GitHub Issue Mirror