-
-
Couldn't load subscription status.
- Fork 523
[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?
[Update] Documentation for sniff WordPress.Files.FilesName #2590
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.
Thanks for working on this PR, @jasonkenison! I left some comments with suggestions. Let me know if you have any questions.
| <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 comment
The 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 ome is incorrectly being flagged as a typo.
| <em>s</em>ome<em>f</em>ile.inc | |
| some<em>-</em>file.inc |
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.
@rodrigoprimo these were tricky, was trying to highlight where non-lowercase letters were used. To get past the spelling error, it now highlights <em>Some</em> as a complete word instead of just the character.
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.
For the valid examples of 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 -template as it is already the case.
For the invalid examples, I think you could highlight just the part that is causing the error.
|
@jasonkenison, I was just wondering if you'll have a chance to finish this off in the near future. It would be great if this PR could be included in the next WPCS release. If you haven't got time or lost interest, please let us know and we'll see if we can find someone to take over. Thanks! |
| <code_comparison> | ||
| <code title="Valid: File name matches prefix and class."> | ||
| <![CDATA[ | ||
| <em>class-bulk-updater</em>.php // For a class named `Bulk_Updater`. | ||
| ]]> | ||
| </code> | ||
| <code title="Invalid: File name missing prefix or class."> | ||
| <![CDATA[ | ||
| class<em>.</em>bulk-updater.php | ||
| <em>bulk-updater</em>.php | ||
| bulk-updater<em>-class</em>.php | ||
| ]]> | ||
| </code> | ||
| </code_comparison> |
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.
@rodrigoprimo I'm not sure how to solve this one, but am happy to discuss or try recommendations.
Regarding your question here, this is more or less what I have in mind. What do you think?
| <code_comparison> | |
| <code title="Valid: File name matches prefix and class."> | |
| <![CDATA[ | |
| <em>class-bulk-updater</em>.php // For a class named `Bulk_Updater`. | |
| ]]> | |
| </code> | |
| <code title="Invalid: File name missing prefix or class."> | |
| <![CDATA[ | |
| class<em>.</em>bulk-updater.php | |
| <em>bulk-updater</em>.php | |
| bulk-updater<em>-class</em>.php | |
| ]]> | |
| </code> | |
| </code_comparison> | |
| <code_comparison> | |
| <code title="Valid: File name matches class name with proper prefix."> | |
| <![CDATA[ | |
| <em>class-bulk-updater</em>.php // `Bulk_Updater`. | |
| <em>class-user-profile</em>.php // `User_Profile`. | |
| <em>class-export-data</em>.php // `Export_Data`. | |
| ]]> | |
| </code> | |
| <code title="Invalid: File name does not match class name or the prefix is incorrect."> | |
| <![CDATA[ | |
| class-<em>update-in-bulk</em>.php // `Bulk_Updater`. | |
| <em></em>user-profile.php // `User_Profile`. | |
| <em>class.</em>export-data.php // `Export_Data`. | |
| ]]> | |
| </code> | |
| </code_comparison> |
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.
@rodrigoprimo Change made in the latest commit
|
@jasonkenison, I reacted with 👍 in the last comment of all the conversations where you already addressed my suggestion. When you have a moment, could you please resolve those? I also replied to your question and provided an example. I believe there is still one comment that was not addressed: #2590 (comment) Thanks for your contributions! |
Related to #1722
Continuing updates from #2492
Closes #2492