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
[Codingstyle] Disallow unused use statements #18520
Conversation
@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. |
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. |
Maybe to prevent it messing up over time we could have either
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? |
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 |
This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers |
@justinvelluppillai let's discuss this one again in one of our next meetings, after 4.8.0-rc1 was released. |
This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers |
This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers |
@justinvelluppillai @bx80 @peterhashair any objections with merging this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be good if we each set up PHPStorm to also do the same
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
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