Squiz/SelfMemberReference: update XML doc

[rodrigoprimo]

Description

This PR updates the XML documentation of the Squiz.Classes.SelfMemberReference sniff to address the following:

• 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.

Suggested changelog entry

Squiz.Classes.SelfMemberReference: improvements to the XML documentation

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

[jrfnl]

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 ?

[jrfnl]

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.

[rodrigoprimo]

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.

[jrfnl]

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 ?

[jrfnl]

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.

[rodrigoprimo]

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>

and that not including 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.

[jrfnl]

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.

[rodrigoprimo]

Thanks for your review, @jrfnl!

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 ?

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.

[jrfnl]

That is a good question. It is not clear to me what coding standards the code examples in the XML documentation should follow.

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 Generic sniffs follow ?

Might be a good idea to open an issue about this for further discussion and decision making.

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?

Yes, the first two examples only have the class ... {} and function {} wrappers to make it valid code, not to demonstrate the issue the sniff is looking for.

For the last example, the class ... {} and function {} wrappers have a functional value to demonstrate the issue.