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

Remove PHPCS javascript linting from themes #2845

Closed
aellison opened this issue Jun 1, 2018 · 4 comments
Closed

Remove PHPCS javascript linting from themes #2845

aellison opened this issue Jun 1, 2018 · 4 comments
Labels
Enhancement A feature or feature request

Comments

@aellison
Copy link

aellison commented Jun 1, 2018

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
Copy link
Contributor

dpagini commented Jun 1, 2018

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
Copy link
Author

aellison commented Jun 1, 2018

@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

@ba66e77 ba66e77 added the Enhancement A feature or feature request label Jun 2, 2018
@dpagini
Copy link
Contributor

dpagini commented Jun 4, 2018

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement A feature or feature request
Projects
None yet
Development

No branches or pull requests

4 participants