search

acquia / blt

Acquia's toolset for automating Drupal 8 and 9 development, testing, and deployment.
https://docs.acquia.com/blt/
GNU General Public License v2.0
442 stars 396 forks source link

Remove PHPCS javascript linting from themes #2845

Closed aellison closed 5 years ago

aellison commented 6 years ago

Out of the box, PHPCS checks all new javascript that is committed to a repo. PHPCS's linter does not seem to be respecting Drupal core's.eslintrc rules and seem to be following PHPCSs' default rules. Cog and most other modern themes ship with their own linters which should be run during the tests:frontend:run command. We've been excluding js linting from individual projects in the phpcs.xml, but I think this should be the default.

dpagini commented 6 years ago

I think this is related to #2824 and I've seen the same thing... *.twig and *.js files are getting PHPCS'd. That was my experience.

aellison commented 6 years ago

@dpagini I think twig files being checked by PHPCS is intentional.

I was also just made aware that PHPCS now has an eslint sniff. I'm experimenting with it now using the drupal core eslint file. As long as it works the same as the node eslint package in cog, it probably a good idea to continue to use PHPCS to check javascript. https://github.com/squizlabs/PHP_CodeSniffer/wiki/Customisable-Sniff-Properties#genericdebugeslint

dpagini commented 6 years ago

Interesting...

So I have a few issues maybe with the implementation though. First, my project is pretty mature at this point... With this new linting, I'm either stuck with a million instances to fix, or I can't enforce this new rule. The sniffing only happens as part of the pre-commit hook, but once the files are committed, CI doesn't care whether it passes linting or not. To date, my project has been successful in implementing code standards b/c if ANY code doesn't pass linting, the entire build fails. Now I can ignore it with a quick $ git commit -n and CI will still pass. This, I think, is part of why I was so confused about the implementation as well. So now I have twig and JS coming up with errors that wouldn't otherwise show up except for when I'm editing those files.

danepowell commented 5 years ago

It sounds like this only applies to new files when PHPCS runs as part of the pre-commit hook, in which case this is a dupe of #3448