-
-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[Documentation] PSR12 - Constant Visiblity #238
[Documentation] PSR12 - Constant Visiblity #238
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 this PR @dingo-d !
Looking good. Just two small questions.
<documentation title="Constant Visibility"> | ||
<standard> | ||
<![CDATA[ | ||
Visibility must be declared on all class constants if your project PHP minimum version supports class constant visibilities (PHP 7.1 or later). |
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.
Is it "class constants" ? Or all OO constants ? Constants can also be declared on interfaces, traits (PHP 8.2+) and enums, after all.
Also not sure if the "class" needs to be repeated.
Visibility must be declared on all class constants if your project PHP minimum version supports class constant visibilities (PHP 7.1 or later). | |
Visibility must be declared on all class constants if your project PHP minimum version supports constant visibilities (PHP 7.1 or later). |
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.
Well, the documentation states
The term "class" refers to all classes, interfaces, and traits.
That's why I opted to use that term.
I added the 'class' to distinguish them from define
d constants.
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.
Well, the documentation states
The term "class" refers to all classes, interfaces, and traits.
That's why I opted to use that term.
I added the 'class' to distinguish them from
define
d constants.
I fully understand (and support) why it was added, I just wonder if the right term was added. After all, the PSR12 definition is not available from within the PHPCS docs.
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.
I've added this to the documentation description, I think it should help to avoid misunderstandings.
src/Standards/PSR12/Docs/Properties/ConstantVisibilityStandard.xml
Outdated
Show resolved
Hide resolved
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 @dingo-d ! LGTM.
Only thing I'm wondering about is whether the "The term "class" refers to all classes, interfaces, and traits." phrase should also mention enums ?
Enums didn't exist when PSR12 was written, but the sniff does apply the rule to enums too.
Good catch! Will update this to add enums as well. |
Add enums to the 'class' list
* Add the documentation for the PSR12 Constant Visiblity sniff
The PR contains the documentation for the PSR12/Properties/ConstantVisiblity sniff.
Description
This PR will add the documentation for the above-mentioned sniff, according to the official standard definitions.
Suggested changelog entry
Add documentation for the PSR12 ConstantVisiblity sniff
Related issues/external references
Part of #148
Types of changes
PR checklist