[Documentation] Squiz: Scope Closing Brace

[jaymcp]

Description

This PR adds documentation for the Squiz.WhiteSpace.ScopeClosingBrace sniff.

Suggested changelog entry

Add documentation for the Squiz Scope Closing Brace sniff

Related issues/external references

Part of #148

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]

@jonmcp Thank you for taking on this sniff !

Aside from the inline notes, I wonder if the code samples could be made more varied ? The sniff is not just about the close brace for function declarations, but also about OO structures and all control structures.

Nitpick: it looks like the CDATA close tag for all code samples is not indented ?

[jrfnl]

@jonmcp Thanks for making those updates!

Regarding the first section: as the code samples are now quite wordy, I'm not sure they each need their own <code_comparison> block. They could possibly be combined in one <code_comparison> block. What do you think ?

Regarding my comments about the second section: The mention of "newline" in the code sample descriptions is misleading as the close brace could be indented.

    if ($something) {
        echo 'close brace is on its own line, but not directly preceded by a new line char';
    }

And the sniff also allows for a mix of PHP and HTML, so the below would also be valid (but not comply with the description as currently in the docs):

    <?php if ($something) { ?>
        <div>sniff regards this as the close brace being on its own line, but the close brace is not directly preceded by a new line char';
    <?php } ?>

Sorry to be a pain, but being precise is kind of important for these things for sniffs.

[jaymcp]

@jonmcp Thanks for making those updates!

Regarding the first section: as the code samples are now quite wordy, I'm not sure they each need their own <code_comparison> block. They could possibly be combined in one <code_comparison> block. What do you think ?

I tried this out locally, and it does look a lot cleaner. I've combined them in 9cd4fd2.

Regarding my comments about the second section: The mention of "newline" in the code sample descriptions is misleading as the close brace could be indented.

That's a very good point. I've combined the conditional with the class declaration in d3e4068 to better illustrate that statements can be indented. I also removed some of the related code comments, not only because they were syntactically invalid, but because I felt that they were making the code block quite messy. I've instead indented the invalid closing braces enough that it should be clear that they are misaligned, rather than relying upon the comments to describe it. I'm not 100% confident that that was the correct decision, so please let me know if you'd like me to work on it further.

And the sniff also allows for a mix of PHP and HTML, so the below would also be valid (but not comply with the description as currently in the docs):

    <?php if ($something) { ?>
        <div>sniff regards this as the close brace being on its own line, but the close brace is not directly preceded by a new line char';
    <?php } ?>

I think I've reworded it to be more clear now. I've also added an example illustrating your code example above in 9cd4fd2.
This may be somewhat unrelated, but the sniff does not catch this scenario:

<?php if ($something) { ?>
    <p>additional whitespace before closing brace</p>
<?php   } ?>

Sorry to be a pain, but being precise is kind of important for these things for sniffs.

Not at all. I agree that it's very important, and I appreciate the time you're taking to thoroughly explain where I have fallen short in my descriptions.

[jrfnl]

This may be somewhat unrelated, but the sniff does not catch this scenario:

<?php if ($something) { ?>
    <p>additional whitespace before closing brace</p>
<?php   } ?>

Good observation. For the above, the sniff does not actually check the indentation of the close brace, but the indentation of the PHP open tag preceding the close tag.

It would flag the code if the PHP open tag preceding the close curly would be indented differently from the start of the line containing the if - and it also determines the needed indentation based on the PHP open tag on the line containing the if.

<?php if ($something) { ?>
    <p>additional whitespace before closing brace</p>
  <?php } ?>

Might be an idea to add an additional check to this sniff to verify that, if a PHP open tag is used to determine whether the indentation is correct, there is only one space between the PHP open tag and the close brace.

If you think that is a good idea, you may want to open an issue for it.

[jrfnl]

I also removed some of the related code comments, not only because they were syntactically invalid, but because I felt that they were making the code block quite messy. I've instead indented the invalid closing braces enough that it should be clear that they are misaligned, rather than relying upon the comments to describe it. I'm not 100% confident that that was the correct decision, so please let me know if you'd like me to work on it further.

@jonmcp I definitely think that was the right decision. I even think some of the remaining wordiness in the code samples could be trimmed down, as the code samples are clear enough as they are IMO, but I'm fine with leaving those as they are. Up to you.

One thing I did notice was that all examples have too much indentation for the close brace, while the sniff would also flag too little indentation. Was that intentional ?

I'm okay with merging the PR as-is, but will give you a little time to think the above over. Let me know how you want to proceed.

where I have fallen short in my descriptions.

Please don't take code reviews personally. You are new to this codebase, so give yourself some leeway. I'm happy to provide guidance and/or answer questions to help you find your feet.

[jaymcp]

Might be an idea to add an additional check to this sniff to verify that, if a PHP open tag is used to determine whether the indentation is correct, there is only one space between the PHP open tag and the close brace.

If you think that is a good idea, you may want to open an issue for it.

I do think that it would be useful; in fact, I think it would be good to catch it either side of the closing brace, but that would perhaps be outside the scope of this sniff. I'll open an issue anyway, to avoid getting too far off topic in this PR. 🙂

I even think some of the remaining wordiness in the code samples could be trimmed down, as the code samples are clear enough as they are… all examples have too much indentation for the close brace…

Re the latter part - that's an oversight on my part. I will modify the code comparisons to also include an example of too little indentation, and I'll also take another look at the wordiness whilst I'm working on that.

I'm happy to provide guidance and/or answer questions to help you find your feet.

Thank you so much! 😊

[jaymcp]

@jrfnl apologies for the delay in actioning the feedback here. I am only able to devote time on Mondays and Fridays due to work commitments.
I've reduced the wordiness of the code examples (let me know if I've reduced it too much!), and added an example of too little whitespace.
I'm not 100% sure whether I should have indented the example of too little whitespace or not, though…

[jrfnl]

... and added an example of too little whitespace. I'm not 100% sure whether I should have indented the example of too little whitespace or not, though…

@jonmcp Just a thought: what about removing the function bar() code sample and adjusting the class_exists('Foo') sample to have too much (two spaces instead of zero) indent for the if close brace and too little (two spaces instead of four) indent for the class close brace ?

Like this:

if (!class_exists('Foo')) {
    class Foo {
  }
  }

You could even consider reversing the expected indents:

if (!class_exists('Foo')) {
    class Foo {
}
    }

Re: the <em> tags for <?php } - would it make things clearer if those encapsuled that part of the code ? As in <em><?php }</em> ?

[jaymcp]

Thanks @jrfnl, it looks a lot better that way, I think, in both cases.
I've also changed the example containing the <p> to use <span> for better output as HTML.

As an aside: after this PR is complete, I'll be updating my committer name from Jon to Jay (my GitHub handle won't be changing just yet, though); I hope that's okay.

[jrfnl]

As an aside: after this PR is complete, I'll be updating my committer name from Jon to Jay (my GitHub handle won't be changing just yet, though); I hope that's okay.

Whatever works for you and makes you feel most comfortable. Be yourself, whomever that may be.

I can't rewrite history for any earlier commits to the repo (of course), but I'll make sure to update my draft changelog notes to use Jay instead of Jon. Is that okay for you ?

[jrfnl]

@jonmcp Jay, thank you for your work on this. Looking good.

I'll rebase the PR and will merge once the build has passed.

[jaymcp]

Whatever works for you and makes you feel most comfortable. Be yourself, whomever that may be.

I can't rewrite history for any earlier commits to the repo (of course), but I'll make sure to update my draft changelog notes to use Jay instead of Jon. Is that okay for you ?

That's perfect, and appreciated. Thank you 💜