-
-
Notifications
You must be signed in to change notification settings - Fork 135
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
Going all in on linting #109
base: master
Are you sure you want to change the base?
Going all in on linting #109
Conversation
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.
Personnally I would chose to stay with PHP-CS-Fixer, as it is a more popular choice, and that's an important parameter when introducing a new dependency. But given that we're talking about a linter, it's not a critical choice, so whatever.
It seems ecs is basically a wrapper for PHP-CS allowing parallel scan which we do not care because the codebase is small and honestly if it takes 200ms or 120ms it's not important.
But if you think it has important features, I mean why not, it's just that I don't immediatly see the advantage of moving to another linter lib instead of tweaking the existing config.
As for []
vs array()
, I'm partial to the long form as I find it easier to spot in the code, but it's a matter of taste.
I like that it makes long lines split in multilines.
ecs wraps both php cs and php cs fixer so technically all I have done is reduce the length of the rules file into one with more groups and no specific rules and made it so the default behaviour is to check and not to fix |
The complexity of the current configuration has required me to remove a specific rule as it is now handled by default and caused a lint failure The comparative simplicity of this PR means we aren't beholden to specific minor decisions made upstream which we don't really care about. |
@willpower232 Do you wish to work on resolving conflicts on this branch, or should this be closed now? |
I'll pick this up at some point, probably in a couple of weeks 🙏 |
I've been using easy-coding-standard to wrap phpcs and php-cs-fixer and radically shorten the configuration which explains all my past confusion around spaces in this project.
I tried to maintain the behaviour of
composer lint
andcomposer lint-ci
in spite of ecs's default behaviour being to not fix.I presume that ecs saw the
inheritdoc
's weren't necessary as everything was typed perhaps? Not sure on that one. phpstan doesn't seem to mind so ¯\_(ツ)_/¯I added in another linter I use which I think just makes sure it is valid PHP rather than applying a style.
I also spotted that the phpstan config could be simplified.
cc @NicolasCARPi that I can't directly invite for a review via github