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

Prevent PHPCS from scanning markdown files. #2824

Closed
wants to merge 3 commits into from

Conversation

aweingarten
Copy link
Contributor

Changes proposed:

  • BLT is scanning twig files which is a headache
  • According to this issue the exclude patterns are necessary to prevent PHPCS from merging. The file lists.

https://www.drupal.org/node/2867601#comment-12075633

@mikemadison13
Copy link
Contributor

@aweingarten you mentioned Twig in the issue but excluded md files in the PR. Was this intentional? Should we also add .twig?

@mikemadison13
Copy link
Contributor

@aweingarten ping?

@aweingarten
Copy link
Contributor Author

aweingarten commented May 25, 2018

@mikemadison13 this was intended just to cover markdown. Title has been updated!

@mikemadison13
Copy link
Contributor

@aweingarten i still only see .md files in the patch. if you want to cover just twig, .twig needs to be added

@@ -34,5 +34,6 @@
<exclude-pattern>*/node_modules</exclude-pattern>
<exclude-pattern>*/vendor</exclude-pattern>
<exclude-pattern>*/default.local.settings.php</exclude-pattern>
<exclude-pattern>*md</exclude-pattern>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a section above which may be a better fit...?

<arg name="ignore" value="*.css,*.md,*.txt"/>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this pattern is too broad. I'd have to verify, but I expect that a directory named shazamd would also have its contents ignored with this. As dpagini pointed out, previous sections use a *.md pattern.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've confirmed by suspicion. With the <exclude-pattern>*md</exclude-pattern> rule, if I put an improperly formatted css file inside a directory named shabamd (or anything else ending in md, phpcs stops seeing the css file. If I then rename the directory to shabam, phpcs sees the css file again.

@dpagini
Copy link
Contributor

dpagini commented May 30, 2018

I just ran into a similar problem with BLT 9.0... It seems like when I have *.js files staged in git, the git commit hook is sniffing those files with PHPCS. I'm not sure why *.js is ignored from the ALL sniff, but the individual file list sniff checks those files.

...I'm wondering if the solution for that would be something similar as this?

@aweingarten aweingarten changed the title Prevent PHPCS from scanning twig files. Prevent PHPCS from scanning markdown files. May 30, 2018
Copy link
Contributor

@ba66e77 ba66e77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rule as written is too broad and will exclude things other than markdown files.

@ba66e77
Copy link
Contributor

ba66e77 commented Aug 31, 2018

It looks to me like the test really needs to be <exclude-pattern>*\.md</exclude-pattern> to get .md files and not get other files/directories ending in md.

@danepowell danepowell added the Enhancement A feature or feature request label Oct 9, 2018
@mikemadison13
Copy link
Contributor

this should be covered in 10.x already, see https://github.com/acquia/blt/blob/10.0.x/phpcs.xml.dist#L11

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

Successfully merging this pull request may close these issues.

None yet

6 participants