[Docs] Add XML doc for Squiz.Classes.ClassFileName sniff #843
Conversation
Adjusts the `<em>` tags in the Squiz.Classes.ClassFileName sniff XML docs to better highlight the parts of the code samples that illustrate the purpose of the sniff.
There was a problem hiding this comment.
Hi @braindawg and welcome to the PHP_CodeSniffer project! Thank you for taking on these documentation tasks.
This PR is looking good and as things are, I'm happy to approve it.
However, while reviewing this PR, I looked at the sniff code and realized the sniff contains a problematic logic error, in that it recommends changing the class name instead of changing the file name.
This is problematic as the file name may be a name which is not a valid symbol name in PHP, which would make the error inactionable.
So, inspired by this PR, I've opened PR #845 to fix this (and make some other small improvements). The pertinent commit is c1cf608
So now I have a conundrum: merge this PR now and update the docs in PR #845 to change "Class name [case] does not match filename [case]." to "Filename [case] does not match class name [case]." or ask you to make that change in this PR and merge this after #845 has been merged.
What would you prefer ? And would you like to review PR #845 considering that you're already familiar with the sniff ?
jrfnl
changed the title
Seems like 6 of one, half a dozen of the other to me, as the same changes need to be made either way. If you have no preference, I'll wait until #845 is merged and then add a commit to update the verbiage as suggested. I'm happy to take a look at #845, too, though it might be a few days before I can get to it. |
Sounds like a plan. Thank you for understanding and I look forward to your review when you find the time 👍🏻 |
|
@braindawg Thanks for reviewing #845. I've now merged that PR. Would you like to update this PR to match ? |
Per PR PHPCSStandards#845, the filename must be modified to match the class name, since class names are much more restrictive than filenames and the inverse is not always possible. This clarifies in the example titles that the fix for the invalid code is to modify the filename rather than the class name.
|
Updated the example titles to reflect that the filename must be changed to match the class name. |
There was a problem hiding this comment.
Thanks for bearing with me and making that update @braindawg ! LGTM. Merging.
* Add XML doc for Squiz\Classes\ClassFileName sniff * [Doc] Emphasize sniff focus for ClassFileName Adjusts the `<em>` tags in the Squiz.Classes.ClassFileName sniff XML docs to better highlight the parts of the code samples that illustrate the purpose of the sniff. * Correct file/class name relation in sniff doc Per PR #845, the filename must be modified to match the class name, since class names are much more restrictive than filenames and the inverse is not always possible. This clarifies in the example titles that the fix for the invalid code is to modify the filename rather than the class name.
Description
Adds XML doc for the
Squiz\Classes\ClassFileNamesniff. This is the only sniff I was able to find that refers to both the filename and the file contents, so I'm open to feedback regarding the representation of the filename within the code samples. I tried several variations, hoping this is clear enough.Suggested changelog entry
Add XML doc for the Squiz.Classes.ClassFileName sniff
Related issues/external references
Partial fix for #148
Types of changes
PR checklist