-
-
Notifications
You must be signed in to change notification settings - Fork 77
Squiz/SelfMemberReference: update XML doc #1108
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: master
Are you sure you want to change the base?
Squiz/SelfMemberReference: update XML doc #1108
Conversation
- Improve sniff description explicitly mentioning where the spaces are checked and adding context as to when the `self` keyword is verified. - Fix the code examples for checking `self` case and spaces around the double colon to ensure they trigger the sniff (the sniff checks if the call is made within a class). - Improve the titles of the code examples related to spacing around double colons. - Remove unnecessary methods from the code examples related to using `self` instead of the class name when referencing a static member.
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.
Hi @rodrigoprimo, thanks for this PR!
I've left two comments inline.
Other than that, I wonder if the first two code comparisons need to have the braces on their own lines - it kind of makes the code samples longer than needed ?
When referencing static members within a class, the self keyword must be used instead of the | ||
current class name, it must be lowercase, and there must be no spaces before or after the double | ||
colon. |
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.
This rephrasing is confusing to me.
"static members within a class" will be interpreted as methods/properties declared with the static
keyword, while that's not something the sniff takes into account.
The static
in the "local static member reference" in the error messages is about static references, not static members.
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.
If I understood your point correctly, I failed to consider that even though calling a non-static member with a static reference causes a warning or fatal error depending on the PHP version, it still works in some versions and, more importantly, it is not something that the sniff checks.
What about "When making static references to class members from within the same class" instead of "When referencing static members within a class"?
The description would become:
When making static references to class members from within the same class, the self keyword must be used instead of the current class name, it must be lowercase, and there must be no spaces before or after the double colon.
Or do you prefer the original approach, and then the final description could be:
The self keyword must be used instead of the current class name, it must be lowercase, and there must be no spaces before or after the double colon.
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.
calling a non-static member with a static reference causes a warning or fatal error depending on the PHP version
Actually, as this sniff is about hierarchy keywords, IIRC this won't be a warning nor a fatal error in any PHP version.
Or do you prefer the original approach
I think I do as it makes it more straight-forward to understand what the sniff expects, though the description does contain multiple rules in one description.
Might be worth splitting up the description to have a separate description for each code sample ?
public static function bar() | ||
{ | ||
} | ||
|
||
public static function baz() |
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.
Why did you choose to remove the bar()
methods ? For this specific error, it kind of makes sense to have the function declaration being referenced available in the code sample (though possibly this could be condensed to a one-liner with the braces on the declaration line).
I also wonder if the static
in the function signatures should be removed, as it could give the impression - same as the updated description -, that this is about static
methods only, while that's not the case.
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.
Why did you choose to remove the bar() methods ?
I opted to remove the bar()
methods to simplify the code example. I also took into consideration that they are not necessary to trigger the sniff (the sniff doesn't check if the method being called exists), that AFAIK similar code examples don't provided the called method like in
PHP_CodeSniffer/src/Standards/Generic/Docs/CodeAnalysis/UselessOverridingMethodStandard.xml
Line 26 in 52dd82b
<em>parent::bar();</em> |
bar()
doesn't create a syntax error (despite making the code not functional).
I also wonder if the static in the function signatures should be removed, as it could give the impression - same as the updated description -, that this is about static methods only, while that's not the case.
That is a good point and something that I hadn't considered. I will update the code example to remove static
from the function signature.
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.
Yes, please update the function signature.
Re: the removed method - I don't see any harm in these being there and it makes it clearer what this is about, so I see no reason why it should be removed.
Note: comparing this to the UselessOverridingMethod
sniff is an invalid comparison. This sniff is about references to methods in the same class, while the UselessOverridingMethod
sniff is about methods being overloaded from another class, so not the same thing.
Thanks for your review, @jrfnl!
That is a good question. It is not clear to me what coding standards the code examples in the XML documentation should follow. I also prefer to have the opening brace on the same line as the function/class signature. Happy to change it if it is something that is ok to change. I can check, but I have a feeling that most of the XML documentation in this repo has the opening braces on their own lines due to the coding standard used in this repository. I'm not sure why you are referring just to the two code comparisons and not all of the code comparisons in the documentation for this standard. Is there a reason that I'm missing? If we are going to change the first two code comparisons, I think we should change the third as well. |
That's a good point and I don't think we have clear guidelines for this (yet). Maybe it should be based on the standard a sniff is in ? But then, what coding standard should the code samples for the Might be a good idea to open an issue about this for further discussion and decision making.
Yes, the first two examples only have the For the last example, the |
Description
This PR updates the XML documentation of the Squiz.Classes.SelfMemberReference sniff to address the following:
self
keyword is verified.self
case and spaces around the double colon to ensure they trigger the sniff (the sniff checks if the call is made within a class).self
instead of the class name when referencing a static member.Suggested changelog entry
Squiz.Classes.SelfMemberReference: improvements to the XML documentation
Types of changes
PR checklist