-
-
Notifications
You must be signed in to change notification settings - Fork 518
[Update] Documentation for sniff WordPress.Files.FilesName #2590
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
base: develop
Are you sure you want to change the base?
Changes from 4 commits
6d1f6bb
4f4f541
a08e092
68a2773
200cdfa
768e4a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,86 @@ | ||||||
<?xml version="1.0"?> | ||||||
<documentation xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||||||
xsi:noNamespaceSchemaLocation="https://phpcsstandards.github.io/PHPCSDevTools/phpcsdocs.xsd" | ||||||
title="File Name Standards" | ||||||
> | ||||||
<standard> | ||||||
<![CDATA[ | ||||||
File name must not be a mimetype sublevel only file names, such as `plain.php`. | ||||||
]]> | ||||||
</standard> | ||||||
<code_comparison> | ||||||
<code title="Valid: Is not a mimetype only file name."> | ||||||
<![CDATA[ | ||||||
<em>taxonomy-term</em>.inc | ||||||
<em>class-test-sample</em>.php | ||||||
<em>other-punctuation</em>.inc | ||||||
]]> | ||||||
</code> | ||||||
<code title="Invalid: Is a mimetype only file name."> | ||||||
<![CDATA[ | ||||||
<em>text-plain</em>.php | ||||||
<em>html</em>.inc | ||||||
<em>richtext</em>.php | ||||||
]]> | ||||||
</code> | ||||||
</code_comparison> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please correct me if I'm mistaken, but I don't think the sniff has this rule. This can be tested using one of the invalid examples provided here to see that it does not trigger an error: $ vendor/bin/phpcs -s --standard=WordPress --sniffs=WordPress.Files.FileName text-plain.php
$ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rodrigoprimo my mistake. The THEME_EXCEPTIONS regex comment on line 43 seemed to indicate it would "not allow for mimetype sublevel only file names". I'll remove it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries. I believe this regex is only used to allow for some exceptions when checking theme files when I believe this should change, and we should include the non-default behavior as well, but that is a separate discussion (just mentioning it to give you more context). |
||||||
<standard> | ||||||
<![CDATA[ | ||||||
Filenames should be all lowercase with hyphens separating words. If `$is_theme` property is set to `true` certain theme specific exceptions are made for underscores. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is something that needs to be improved in how sniffs are documented, but currently we only mention the default behavior of the sniff, so I don't think we should mention what happens in
Suggested change
|
||||||
]]> | ||||||
</standard> | ||||||
<code_comparison> | ||||||
<code title="Valid: Lowercase and `-` separators."> | ||||||
<![CDATA[ | ||||||
taxonomy<em>-</em>my<em>-</em>term.inc | ||||||
<em>s</em>ome<em>f</em>ile.inc | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Considering the invalid example related to this one, I believe the valid example should separate the two words with a hyphen, no? This would also fix the error in CI that is a false positive in my opinion, as
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rodrigoprimo these were tricky, was trying to highlight where non-lowercase letters were used. To get past the spelling error, it now highlights There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the first two errors, as they check if the whole name matches what is expect, maybe we can highlight the full name (excluding the period and the file extension)? For the third error, I think it is better to highlight just |
||||||
<em>class</em>-testsample.inc | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if I understand this specific example. Why is |
||||||
other<em>-</em>punctuation.inc | ||||||
]]> | ||||||
</code> | ||||||
<code title="Invalid: Other punctuation or mixed case."> | ||||||
<![CDATA[ | ||||||
taxonomy-my<em>_</em>term.inc | ||||||
<em>S</em>ome<em>F</em>ile.inc | ||||||
<em>testsample</em>-Unit2.inc | ||||||
other<em>+</em>punctuation<em>#</em>.inc | ||||||
]]> | ||||||
</code> | ||||||
</code_comparison> | ||||||
<standard> | ||||||
<![CDATA[ | ||||||
Class file names should be based on the class name with `class-` prepended. | ||||||
]]> | ||||||
</standard> | ||||||
<code_comparison> | ||||||
<code title="Valid: File name matches prefix and class."> | ||||||
<![CDATA[ | ||||||
<em>class-bulkupdater</em>.php | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It depends on the class name, but considering that the idea is to separate words using hyphens and also considering the name of the corresponding invalid example, maybe this one should be Related to the above, maybe you can add a code comment with the class name to those examples? And then include an example where the name is invalid because the file name does not match the class name, despite it following the other formatting rules? Then it might also make sense to include a valid example for each of the invalid examples. Maybe there should be one invalid example without the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rodrigoprimo might be unrelated to this sniff, but if a class is called "BulkUpdater" this would still include a '-' in the filename? Updated as requested, but can make adjustments as needed. Examples elsewhere in wp-includes, showing 3 instances where these code standard rules aren't followed:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@jasonkenison, good question. If the class is called
You can run PHPCS to test it. For example, given the following file named <?php
class BulkUpdater {} We get the following error when running this sniff:
I was unable to find
|
||||||
]]> | ||||||
</code> | ||||||
<code title="Invalid: File name missing prefix or class."> | ||||||
<![CDATA[ | ||||||
<em>class.</em>bulk-updater.php | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if here it is not better to highlight just the dot as this is the exact part that would cause the error?
Suggested change
|
||||||
<em>bulkupdater-class</em>.php | ||||||
class-<em>CustomFormatter</em>.php | ||||||
]]> | ||||||
</code> | ||||||
</code_comparison> | ||||||
<standard> | ||||||
<![CDATA[ | ||||||
Files in `wp-includes` containing template tags should end in `-template`. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this description could detail a bit more which files in the
Suggested change
|
||||||
]]> | ||||||
</standard> | ||||||
<code_comparison> | ||||||
<code title="Valid: File name with proper prefix/suffix."> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe remove |
||||||
<![CDATA[ | ||||||
general<em>-template</em>.php | ||||||
]]> | ||||||
</code> | ||||||
<code title="Invalid: File name missing prefix/suffix."> | ||||||
<![CDATA[ | ||||||
<em>general</em>.php | ||||||
]]> | ||||||
</code> | ||||||
</code_comparison> | ||||||
</documentation> |
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.
The title of the document should match the sniff name.