Skip to content

Conversation

@Flyingmana
Copy link
Contributor

covers PHPCompatibility.UseDeclarations.NewGroupUseDeclarations and PHPCompatibility.UseDeclarations.NewUseConstFunction.

I can also split it up if needed.

For NewGroupUseDeclarations I was not sure how to properly split the 2 cases up, so I stayed with the cross compatible variant for both.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

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 documentation attributes 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 for use function on the left start on the same line as the group use statement for use function on the right etc.

Suggestions:

  • I'd suggest not using the namespace keyword as part of the namespaces used in the code samples as that is not allowed prior to PHP 8.0.
    Maybe change some\namespace to My\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 with fn having 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 documentation attributes 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 the echo statements on the left start on the same line as the echo statements 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 ?

@jrfnl
Copy link
Member

jrfnl commented Apr 13, 2023

@Flyingmana Just checking - was there anything in the review which wasn't clear ? Will you be able to continue with this PR ?

@Flyingmana
Copy link
Contributor Author

Hi, it will take a few weeks before I can continue with it. Just moved and have everything still in boxes and no working internet yet.

I think everything was clear, but will read closer over it when my Internet is working again.

@jrfnl
Copy link
Member

jrfnl commented Apr 13, 2023

@Flyingmana Understood. Congrats on the new home and good luck with the unpacking. (which reminds me... I still have some unpacked boxes standing around... from my last move a dozen years ago.... 😱 )

@jrfnl
Copy link
Member

jrfnl commented Aug 4, 2023

@Flyingmana How's the new home ? All settled in ? Presuming you got working internet by now, I'd love to see this PR updated so we can get it merged ;-)

@Flyingmana
Copy link
Contributor Author

new home is great, and internet also working, just my life constantly keeping me busy 😅
will take a few more weeks as how life is, Iam getting married tomorrow 😊

(if someone else takes it over, its also ok for me)
But else I promise to continue on this PR "soon" 😅

@jrfnl
Copy link
Member

jrfnl commented Aug 5, 2023

new home is great, and internet also working, just my life constantly keeping me busy 😅 will take a few more weeks as how life is, Iam getting married tomorrow 😊

Blimey! Congratulations! Wishing you a great day tomorrow!

@Flyingmana Flyingmana force-pushed the docs-use-declaration branch from 251d32b to 014e09c Compare November 5, 2023 23:08
@Flyingmana
Copy link
Contributor Author

I think I resolved most of the issues now, possible I missed something.

for NewUseConstFunction I added a placeholder in the example to have the actual changed lines on the same line, as without the cli output did truncate the empty lines in the beginning

----------------------------------------- CODE COMPARISON ------------------------------------------
| Cross-version compatible: Using constants and  | PHP >= 5.6: Importing constants and functions   |
| functions without importing them into the      | into a namespace using `use function` and `use  |
| namespace.                                     | const` declarations.                            |
----------------------------------------------------------------------------------------------------
| // placeholder for line numbers                | use const My\Project\ConstA;                    |
|                                                | use function My\Project\fn_a;                   |
|                                                |                                                 |
| echo My\Project\ConstA;                        | echo ConstA;                                    |
| echo My\Project\fn_a();                        | echo fn_a();                                    |
----------------------------------------------------------------------------------------------------

@jrfnl
Copy link
Member

jrfnl commented Oct 28, 2024

@Flyingmana I'm so sorry, this one fell through the cracks again review-wise. There are still some tweaks I'd like to make before merging it. Would you mind if I make those myself on your PR ? Or would you prefer I leave notes inline ?

@Flyingmana
Copy link
Contributor Author

no problem, if you can do the changes directly I would be very thankful 😊
else its probably again taking a while 😅

@jrfnl jrfnl added this to the 10.0.0 milestone Sep 1, 2025
@jrfnl
Copy link
Member

jrfnl commented Sep 1, 2025

@Flyingmana Pff.. my apologies, making those updates still kept falling through the cracks. I've done them now though and enabled the CI run. Once the build passes, I will merge this PR.

@jrfnl jrfnl enabled auto-merge (squash) September 1, 2025 17:52
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

Thank you for contributing @Flyingmana ! Glad this will be merged now.

And if you'd be up for it, there are plenty more sniffs looking for docs, so I look forward to future PRs ;-)

@jrfnl jrfnl merged commit b3ebdbc into PHPCompatibility:develop Sep 1, 2025
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants