Extra: add sniff to discourage the use of "long" closures

[jrfnl]

Loosely related to the discussion in #1486.

This adds a new sniff to the WordPress-Extra ruleset to discourage (warning) the use of "long" closures.

By default, the sniff will warn when a closure exceeds 5 lines and throw an error when the closure exceeds 8 lines, though these numbers are configurable. By default, the sniff will only count code lines when determining the closure length and will ignore comment-only and blank lines.

As the intention is to only discourage long closures, the error has been disabled (by setting it to a really high value).

Ref: PHPCSStandards/PHPCSExtra#240

---

👉 To discuss: should the value for the recommendedLines property stay at 5 (sniff default) or do we want to use a different value ?

Some input for this based on a run against WP Core (even though the sniff is being added for `Extra` not `Core`):

// Stats (metrics) for closure use in WP Core `src`:
Closure length (code only):
        1 lines  => 50 ( 56.18%)
        2 lines  =>  4 (  4.49%)
        3 lines  =>  7 (  7.87%)
        4 lines  =>  4 (  4.49%)
        5 lines  =>  3 (  3.37%)
        6 lines  =>  1 (  1.12%) // Warnings would start from here.
        7 lines  =>  4 (  4.49%)
        8 lines  =>  4 (  4.49%)
        9 lines  =>  1 (  1.12%)
        10 lines =>  3 (  3.37%)
        13 lines =>  1 (  1.12%)
        14 lines =>  1 (  1.12%)
        15 lines =>  1 (  1.12%)
        17 lines =>  1 (  1.12%)
        23 lines =>  1 (  1.12%)
        36 lines =>  1 (  1.12%)
        45 lines =>  1 (  1.12%)
        53 lines =>  1 (  1.12%)
        -------------------------
        total    => 89 (100.00%)

// Stats (metrics) for closure use in WP Core `tests`:
Closure length (code only):
        0 lines  =>    2 (  0.78%)
        1 lines  =>  120 ( 47.06%)
        2 lines  =>   37 ( 14.51%)
        3 lines  =>   26 ( 10.20%)
        4 lines  =>   20 (  7.84%)
        5 lines  =>   10 (  3.92%)
        6 lines  =>    3 (  1.18%) // Warnings would start from here.
        7 lines  =>   11 (  4.31%)
        8 lines  =>    9 (  3.53%)
        9 lines  =>    2 (  0.78%)
        10 lines =>    7 (  2.75%)
        11 lines =>    2 (  0.78%)
        12 lines =>    1 (  0.39%)
        13 lines =>    1 (  0.39%)
        14 lines =>    1 (  0.39%)
        20 lines =>    1 (  0.39%)
        25 lines =>    1 (  0.39%)
        27 lines =>    1 (  0.39%)
        ---------------------------
        total    =>  255 (100.00%)

[GaryJones]

This feels more subjective than other items in Extra, even with the option of recommendedLines.

What do WordPress plugin and theme authors have to gain by having closures limited to a certain number? Why 5, other than it's half of a round number? Is there any external research that says there is a fundamental benefit to reading code with closures that are 5 (or X) lines or less? (Nothing wholly relevant in https://phpmd.org/rules/codesize.html that we could align to.)

[jrfnl]

This feels more subjective than other items in Extra

Precisely the reason why it is a separate PR ;-)

What do WordPress plugin and theme authors have to gain by having closures limited to a certain number?

Personally, my primary argument would be that it improves the testability of code.

Closures cannot be tested on their own and long closures imply higher complexity of the code.
In other words, a code block which includes a long closure becomes infinitely harder to test as the tests now have to cover all code paths in both the base code + the closure. Separating the closure out to a named function makes the closure testable on its own and lowers the number of tests needed overall.

Another argument for turning long closures into named functions is documentation/knowledge transfer.

Generally speaking, most people do not add a docblock to a closure declaration and documentation related sniffs do not flag this either, while if a closure has higher complexity, it would often be good to document the what and why. Named function declarations lend themselves better for this and we have sniffs in place to enforce docblocks for named functions.

Why 5, other than it's half of a round number?

5 is an arbitrary default, though it was chosen with the above mentioned testability argument in mind. (the default for maxLines in the sniff is 8)

Is there any external research that says there is a fundamental benefit to reading code with closures that are 5 (or X) lines or less?

Readability was not the reason I created the sniff, so I honestly don't know. I didn't do a search to find any readability related research.

And while, this PR doesn't solve #1486, it is related to the discussion in that ticket, in particular this bit:

The general problems I'd like to address with this ticket are that:

• Anonymous functions miss context. A function's name (usually) gives pretty good idea of that a function does.
• Very often anonymous functions miss a docblock or any inline documentation. This is primarily caused by the code formatting/layout.
• Anonymous functions usually add redundant levels of indentation that can reduce readability.

Ref: #1486 (comment)

[GaryJones]

✅ Approving but this may need a review in the future.

[jrfnl]

✅ Approving but this may need a review in the future.

Could you indicate for what ? I mean we are currently reviewing, so I'm happy to discuss any concerns you have.

And yes, I do suspect we'll probably get some complaints about this sniff. Still, I think I made the reasoning for adding it clear.
And if needs be, end-users can adjust the property values to their liking via their own custom ruleset and just like every other sniff, the sniff can also be excluded completely via a custom ruleset (or even via CLI).

[dingo-d]

I agree with the reasoning Juliette added, but I'm a bit on the fence about adding this, as it seems a bit arbitrary. I'm not sure if there is a 'good' value for closure length 🤷🏼‍♂️
But it can easily be turned off in custom rulesets so it solves any issues that people might have (although I doubt we'll have a lot of those).

[jrfnl]

Saying "no" to this proposal is perfectly acceptable, but in that case, I do think this needs to be further discussed in #1486 as this sniff proposal is linked to that issue.

[dingo-d]

I never saw a make post about discouraging 'long' closures to be honest, and since we should always create make posts for these kinds of things, I'd be willing to say no until the issue is agreed upon and then we can just easily add the rule in 3.x version of WPCS.

[jrfnl]

I never saw a make post about discouraging 'long' closures to be honest, and since we should always create make posts for these kinds of things, I'd be willing to say no until the issue is agreed upon and then we can just easily add the rule in 3.x version of WPCS.

Make posts are for things which go into the Core rules, not for Extra.

Having said that, this does (loosely) relate to the following section in the PHP Coding Standards: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#closures-anonymous-functions and yes, IIRC that section was added without consultation nor a Make post (and not by us).

[GaryJones]

Having re-read 1486, and the associated trac ticket, I'm now more unsure.

While I agree that smaller closures have the benefits outlined, and that's generally a good practice, I'm not convinced that it's WPCS's place to push that in this case. We don't have recommendations for function or class lengths, or cyclomatic complexity, and making a suggestion on the number of lines in a Closure feels like those.

Like the deprecation notice on https://github.com/object-calisthenics/phpcs-calisthenics-rules says:

PHP_CodeSniffer is great for handling spaces and char positions. Yet these rules are about code architecture and structure. In 2020, there is tool that suits this perfectly - PHPStan.

[jrfnl]

Like the deprecation notice on https://github.com/object-calisthenics/phpcs-calisthenics-rules says:

PHP_CodeSniffer is great for handling spaces and char positions. Yet these rules are about code architecture and structure. In 2020, there is tool that suits this perfectly - PHPStan.

That notice is unrelated and IMO ill-informed. I mean nearly all WPCS native sniffs are non-code style sniffs, so in what way is PHPCS "only good for handling space and char positions" ?

[jrfnl]

We asked for feedback a month ago and haven't received any. What do we want to do with this PR ? Merge it or move it out of the 3.0.0 milestone ? (both are valid options)

[GaryJones]

As it's not clear cut, let's leave this one for now.

[jrfnl]

As it's not clear cut, let's leave this one for now.

I've moved the PR to the "Future release" milestone now.