[xml doc] NewGroupUseDeclarations and NewGroupUseConstFunction by Flyingmana · Pull Request #1470 · PHPCompatibility/PHPCompatibility
Hi @Flyingmana, thank you for your willingness to contribute!
I'm sorry it took a while before I got to reviewing the PR, but I was on a break for most of January/February and am still playing catch-up now.
To start with your questions:
I can also split it up if needed.
I'm fine with leaving this PR as-is, but would prefer one PR per doc moving forward as that way when one doc still needs fixes, it won't block other docs from being merged in.
For NewGroupUseDeclarations I was not sure how to properly split the 2 cases up, so I stayed with the cross compatible variant for both.
You can use multiple <standard> blocks in the XML docs, each with their own set of <code_comparion>s.
As that particular sniff is looking for two distinct changes, I believe that would be the way to go for that sniff.
For the second part (the trailing comma's), I'd expect the "valid" code sample to show a group use statement without a trailing comma.
For the "valid" code sample description, I'd suggest something like "PHP >= 7.0: Group Use Declarations without trailing commas."
PR review
I've reviewed the docs in detail. Please find my feedback below.
I hope the review helps and is clear enough. Please let me know if you have any questions.
To start, the directory name used in the Docs folder does not match the directory name used in the Sniffs folder, which means the docs are not accessible from the command line (at all).
Please change the subdirectory name in the Docs folder to UseDeclarations to make the docs usable.
Docs review checklist for PHPCompatibility.UseDeclarations.NewGroupUseDeclarations
- ✔️ Only space indentation is used.
- ⚠️ Indentation is consistent - with the exception of the indentation for the
documentationattributes and closer. - ✔️ The title matches the sniff name (or is close enough).
- ❌ Separate error codes have a separate
<standard>block with their own code samples.
(see my answer to your question about this above) - ⚠️ The "standard" description explains sufficiently what was changed in PHP.
Let me know if you'd like a suggestion on how to improve this. - ✔️ The "standard" description uses proper punctuation.
- ✔️ Code sample descriptions use correct prefixes.
- ✔️
<and>are encoded in the code sample descriptions. - ⚠️ The code sample descriptions are descriptive enough.
See my suggestion above for the second "valid" example. - ✔️ Code sample descriptions use proper punctuation.
- ⚠️ The code samples demonstrate the issue sufficiently.
See my suggestion above for the second "valid" example. - ❌ The code samples are valid PHP code.
- ❌
<em>tags are used to highlight in the code samples what the sniff is looking. - ❌ The line length of the code samples stays within the character limit (48 chars).
- ❌ The readability of the code samples is good.
I'd like to suggest formatting the group use statements as multi-line.
This is the more common way of formatting those anyway and it improves the readability and prevents the line length issue.
I'd also like to suggest using blank lines in the code samples to allow the left and right examples to match up, i.e. have the code sample foruse functionon the left start on the same line as the group use statement foruse functionon the right etc.
Suggestions:
- I'd suggest not using the
namespacekeyword as part of the namespaces used in the code samples as that is not allowed prior to PHP 8.0.
Maybe changesome\namespacetoMy\Project?
Also note the change in capitalization to more closely match the conventions for namespace names. - I'd suggest not using the
fn_*function names as it may be confusing to people what withfnhaving become a reserved keyword for arrow functions in PHP 7.4. - Maybe use uppercase names for the constants ? (as that is more common/recognizable)
- Maybe use class/function/constant names which look like this could be real code ?
Docs review checklist for PHPCompatibility.UseDeclarations.NewUseConstFunction
- ✔️ Only space indentation is used.
- ⚠️ Indentation is consistent - with the exception of the indentation for the
documentationattributes and closer. - ⚠️ The title matches the sniff name (or is close enough).
See inline comment. - ✔️ Separate error codes have a separate
<standard>block with their own code samples. - ⚠️ The "standard" description explains sufficiently what was changed in PHP.
See inline comment. - ✔️ The "standard" description uses proper punctuation.
- ✔️ Code sample descriptions use correct prefixes.
- ✔️
<and>are encoded in the code sample descriptions. - ⚠️ The code sample descriptions are descriptive enough.
See inline comments. - ⚠️ Code sample descriptions use proper punctuation.
See inline comments. - ⚠️ The code samples demonstrate the issue sufficiently.
See suggestions below. - ❌ The code samples are valid PHP code.
- ❌
<em>tags are used to highlight in the code samples what the sniff is looking. - ✔️ The line length of the code samples stays within the character limit (48 chars).
- ⚠️ The readability of the code samples is good.
I'd like to suggest using blank lines in the code samples to allow the left and right examples to match up, i.e. have theechostatements on the left start on the same line as theechostatements on the right etc.
Suggestions:
- Same remarks as I made for the other doc about the namespace used and the function/constant names.
- Maybe add a example to each with a non-namespaced native PHP funcion/constant being imported ?
- Maybe add a namespace declaration to the code samples ?