search

Automattic / VIP-Coding-Standards

PHP_CodeSniffer ruleset to enforce WordPress VIP coding standards.
https://wpvip.com/documentation/how-to-install-php-code-sniffer-for-wordpress-com-vip/
Other
236 stars 40 forks source link

Rulesets: move VariableAnalysis configuration from Go to Minimum #763

Closed jrfnl closed 1 year ago

jrfnl commented 1 year ago

While looking into #588, I noticed something which seemed odd, so I'm addressing this with this PR.

The configuration which was added in #690 (and addressed #588) was added to the WordPress-VIP-Go ruleset, not the WordPressVIPMinimum ruleset from which the WordPress-VIP-Go ruleset inherits.

I can not think of any pertinent reason why the configuration should be added for one, but not the other and I couldn't find any discussion on this either, so I propose moving the configuration from the WordPress-VIP-Go ruleset to the WordPressVIPMinimum ruleset.

Ref: https://github.com/sirbrillig/phpcs-variable-analysis/releases/tag/v2.9.0

GaryJones commented 1 year ago

Nobody should be using WordPressVIPMinimum standard at all, so putting it in the WordPress-VIP-Go just made it a little more visible to users who looked at the ruleset file (though the full picture does need both ruleset files).

What advantage does having it in the WordPressVIPMinimum give?

If the name of the WordPress-VIP-Go ruleset didn't change for 4.x, then the plan would be to move everything from WordPressVIPMinimum into the newer ruleset, and then eliminate obvious immediate overrides.

jrfnl commented 1 year ago

Nobody should be using WordPressVIPMinimum standard at all, so putting it in the WordPress-VIP-Go just made it a little more visible to users who looked at the ruleset file (though the full picture does need both ruleset files).

What advantage does having it in the WordPressVIPMinimum give?

Well, some plugin devs may be (= are) using VIPCS outside of the VIP CI context, for those, it maybe useful.

Also, if you look at the rulesets, Minimum basically contains the majority of the "base" config, while Go builds on top of that and customizes. As this is "base" config, it just feels like something which belongs in Minimum, not necessarily Go (other than if Go would add/change the config).

If the name of the WordPress-VIP-Go ruleset didn't change for 4.x, then the plan would be to move everything from WordPressVIPMinimum into the newer ruleset, and then eliminate obvious immediate overrides.

WordPress-VIP-Go is not a name for a standard which can contain sniffs (as it contains - dashes, which can't be used in a namespace).

If I remember correctly, a potential ruleset reorganisation is being discussedin #600. Probably best to keep the discussion in that issue. I've moved the ticket to the 4.x milestone now for higher visibility/findability.

GaryJones commented 1 year ago

Well, some plugin devs may be (= are) using VIPCS outside of the VIP CI context, for those, it maybe useful.

Even if they are, they should still be using the WordPress-VIP-Go.

In this case, the changes for the variable analysis config is far more targeted to theme developers.

jrfnl commented 1 year ago

In this case, the changes for the variable analysis config is far more targeted to theme developers.

Good point. Either way, this change doesn't do any harm.