PHPCSStandards / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
880 stars 54 forks source link

[Documentation] Squiz: Function Spacing #451

Open jaymcp opened 5 months ago

jaymcp commented 5 months ago

Description

This PR adds documentation for the Squiz.WhiteSpace.FunctionSpacing Sniff.

Suggested changelog entry

Add documentation for the Squiz Function Spacing Sniff

Related issues/external references

Part of https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/148

Types of changes

PR checklist

jaymcp commented 5 months ago

@jrfnl apologies for the delay since my last PR. FYI my GitHub handle has now changed (jonmcp => jaymcp).

jaymcp commented 4 months ago

Thanks @jrfnl!

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 ?

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.

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 decided on that purely because they have different error codes. Would you like me to combine them?

jrfnl commented 4 months ago

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 ?

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.

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)

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 decided on that purely because they have different error codes. Would you like me to combine them?

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)

jrfnl commented 4 months ago

@jaymcp Just checking in: had a chance to think my question over yet ?

jaymcp commented 3 months ago

Apologies for the further delay, @jrfnl, and thanks for your continued patience.

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 ?

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.

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)

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.

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 decided on that purely because they have different error codes. Would you like me to combine them?

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)

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. I now realise, though, that I've not included an example similar to that shown in one of the unit tests.