[Documentation] Squiz: Function Spacing #451
Conversation
|
@jrfnl apologies for the delay since my last PR. FYI my GitHub handle has now changed ( |
There was a problem hiding this comment.
LGTM,
For reference the added documentation renders as follows:
-------------------------------------------
| SQUIZ CODING STANDARD: FUNCTION SPACING |
-------------------------------------------
There should be exactly 2 blank lines before a function within a statement group or control
structure when it is the first block of code.
----------------------------------------- CODE COMPARISON ------------------------------------------
| Valid: Exactly 2 blank lines before the first | Invalid: Too few blank lines before the first |
| function. | function. |
----------------------------------------------------------------------------------------------------
| class MyClass | class MyClass |
| { | { |
| | |
| | public function foo() { |
| public function foo() { | } |
| } | |
| | |
| | } |
| } | |
----------------------------------------------------------------------------------------------------
----------------------------------------- CODE COMPARISON ------------------------------------------
| Valid: Exactly 2 blank lines before the first | Invalid: Too many blank lines before the first |
| function. | function. |
----------------------------------------------------------------------------------------------------
| if ($something) { | if ($something) { |
| | |
| | |
| function foo() { | |
| } | function foo() { |
| | } |
| | |
| function bar() { | |
| } | function bar() { |
| | } |
| | |
| } | |
| | } |
----------------------------------------------------------------------------------------------------
There should be exactly 2 blank lines before and after a function declaration.
----------------------------------------- CODE COMPARISON ------------------------------------------
| Valid: Exactly 2 blank lines before and after | Invalid: Incorrect number of blank lines |
| function declarations. | before/after function declarations. |
----------------------------------------------------------------------------------------------------
| interface MyInterface | interface MyInterface |
| { | { |
| | |
| | |
| public function foo(); | public function foo(); |
| | public function bar(); |
| | |
| public function bar(); | |
| | |
| | public function baz(); |
| public function baz(); | |
| | |
| | } |
| } | |
----------------------------------------------------------------------------------------------------
There should be exactly two blank lines after a function within a statement group or control
structure when it is the last block of code.
----------------------------------------- CODE COMPARISON ------------------------------------------
| Valid: Exactly 2 blank lines after the last | Invalid: Too few blank lines after the last |
| function. | function. |
----------------------------------------------------------------------------------------------------
| trait MyTrait | trait MyTrait |
| { | { |
| | |
| | |
| public function foo() { | public function foo() { |
| } | } |
| | |
| | |
| public function bar() { | public function bar() { |
| } | } |
| | } |
| | |
| } | |
----------------------------------------------------------------------------------------------------
----------------------------------------- CODE COMPARISON ------------------------------------------
| Valid: Exactly 2 blank lines after the last | Invalid: Too many blank lines after the last |
| function. | function. |
----------------------------------------------------------------------------------------------------
| if ($something) { | if ($something) { |
| | |
| | |
| function foo() { | function foo() { |
| } | } |
| | |
| | |
| function bar() { | function bar() { |
| } | } |
| | |
| | |
| } | |
| | } |
----------------------------------------------------------------------------------------------------
There was a problem hiding this comment.
@jaymcp Hi Jay, welcome back and thanks for this PR.
Generally speaking, looking good again !
I have a couple of questions. These don't necessarily mean anything needs to change, I'd just like to hear your reasoning and we can discuss further based on that.
- In the docs, it mentions "within a statement group or control structure" in a couple of places. Where does this come from ? And is this clear enough ?
- While the sniff allows for configuring different expectations for the spacing before/after the first/between/last function, in its default state, the expectations are the same: 2 blank lines. Have you considered combining the standards/code samples ? And if so, what made you decide against that ?
I look forward to seeing your reply.
|
Thanks @jrfnl!
I struggled with the wording of this; I couldn't think of a reasonably concise way of explaining the expectations of this rule in a way that also provided enough context. I landed on "statement group", but I agree that it's not clear enough. I'd appreciate your advice here.
I decided on that purely because they have different error codes. Would you like me to combine them? |
Well, let's break it down: what are you trying to say with it ? (in your own words, doesn't have to ready for the docs, just trying to get a feel for why the phrase was added)
Not necessarily, though I wonder if the amount of examples is overkill. Can you think of a way to still keep the gist of the code samples intact with a more minimal set of examples ? (and "no", is, of course, also a potential answer) |
|
@jaymcp Just checking in: had a chance to think my question over yet ? |
|
Apologies for the further delay, @jrfnl, and thanks for your continued patience.
I was trying to articulate that, not only will this sniff check functions in the top-level of a file and in perhaps more obvious places like object structures, but also in control structures and whatnot. I could perhaps have said something like "There should be exactly 2 blank lines before/after a function declaration.", but wasn't 100% confident that that was accurate either.
We could probably remove the first and last code samples without impacting readability at all, in my opinion. I wanted to illustrate that this sniff did not only apply to object structures, but we don't need examples of all of them, and perhaps not more than one example for a conditional. |
Description
This PR adds documentation for the
Squiz.WhiteSpace.FunctionSpacingSniff.Suggested changelog entry
Add documentation for the Squiz Function Spacing Sniff
Related issues/external references
Part of #148
Types of changes
PR checklist