Proposal for restructuring the rulesets & some of the sniffs #1157
Comments
|
👍 For proposal 2. I've thought before that things should be better organized, and although the BC breaks will be a pain, that's just another reason to do it sooner rather than later. For proposal 1, I favor the general idea, but I'm not sure I understand the necessity of having I think if we do these, dedicating a release to them is definitely a good idea. |
I was thinking that having a separate [Edit]: Maybe all non-blocking modern sniffs could just go into |
That's what I was thinking as well. In general, it would be nice if the standard automatically progressed as a project evolved to using newer PHP features, rather than having to remember manually switch standards. (Though the blocking sniffs would unfortunately still need to isolated, I guess.)
This makes me think that possibly people will want different levels of modernity. I mean, some people will want namespaces long before they want to mandate type declarations. And fragmenting this further into sub-standards doesn't really seem like a good idea. So maybe we need to think about this some more. In addition, I was also thinking that it might be better to use |
|
Thumbs up for proposal 2 👍 For proposal 1 also a thumbs up 👍 though I'm not a fan of _If I were a new user who has just discovered this repo after a quick glance I'd more than likely skim read the docs I'm pretty sure I might just go with So, rather than using a time methodology to name these how about swapping out
And change the newly introduced compound ruleset from ESLint also has a category of rules "Possible Errors" along with "Best Practices" and "Stylistic Issues" Would it be worth considering moving the relevant sniffs to a |
|
@ntwb Thanks for your response and your thoughts. I'm not a fan of the "Next" terminology as in
There are a number of sniffs which point out possible errors, but for WPCS, those are generally thrown as |
|
Lately, I am working more and more with PHPCS and WPCS, for example, for MultilingualPress (also the current stable legacy branch - ugh). Furthermore, I have been following discussion both here and in the Slack channel for a little while. Now, I would like to get involved here (more and more). 🙂 👋 In general, I like both proposals, or rather: the individual idea behind them. I fully support the split between Stylistic Issues and Best Practices (which also happen to be ESLint categories, as already mentioned before). Also, having a VIP-specific ruleset, To be honest, I never (and here I am not talking only about the last weeks, because I had been working with PHPCS and WPCS already several times before) understood the Furthermore, I neither support I would rather have something like Next, I would not have something like
I disagree. In my opinion, WPCS should follow state-of-the-art best practices. If people do not want or are not able to do this, they are free to either choose the legay variant, if it exists, and include rules as they see fit, or start with the state-of-the-art ruleset and then exclude stuff at will. Summing up:
°: both excluding (too modern) rules, and including/redefining rules. Re: Proposal 2: Moving the mentioned sniffs into (new) categories makes perfect sense to me. 👍 Also, applying the changes in a dedicated release without doing anything else makes a lot of sense to me. Also, not sure I am stirring up a hornet's nest here, is there any reason the package is (still) in major version zero? I'd say, it's (way beyond) time to release, Last but not least:
While I agree that PSR-2 plugins/developers exist, and thus making life easier for them is a nice thing to be able to offer, I am in the PSR-4 camp, which has problems with the filename-specific sniffs/messages. Thanks for reading. Cheers, |
I am happy to see new use cases rise with increased adoption and I fully support increasing flexibility to accommodate them. However I find multiple issues with specific suggested expansion, especially:
To accommodate described use cases my first thought was — subsplit I think Proposal 1 👎 as currently written, 👍 for valid use cases to improve for |
|
👋 @tfrommen Thanks for weighing in on this, and it is great to hear that you'd like to get more involved! I like your idea of In regard to 1.0, this was last discussed in #718, but really should have a dedicated issue (previously #519, but we should open a new one). |
|
The original proposal didn't mention this, but IMO the |
|
@tfrommen @Rarst Thank you both for adding your thoughts. Based on the comments so far regarding including versus excluding sniffs, I think I need to clarify something about how rulesets work: A ruleset is read from top to bottom and each standard/sniff/directive is added to PHPCS in order. That also means that standards with conflicting directives - i.e. standard 1 excludes sniff A and error code B, while standard 2 includes sniff A and the sniff where errorcode B comes from, will lead to unpredictable results, or rather, will make the loading order of standards something everyone would need to learn about in detail to create their custom rulesets. The short of it is: it's much easier to work with standards which include rules, than with standards which exclude rules/errorcodes. In addition to this, while PHPCS 3.x supports including a single errorcode from a sniff, PHPCS 2.x does not and WPCS has chosen to continue support for PHPCS 2.x until summer 2018. The above include vs exclude logic, has, of course, played a large part in how I came to the current proposal for the ruleset split. |
Yes, I realized late that I didn't mention that ruleset in the original post. IMHO the There are two main reasons for that:
|
|
@jrfnl I can't think of a single time this had been an issue for me in practice. Could you please provide a specific example maybe? For the record I think my rulesets tend to be quite exclusion–centric, as it makes sense to me, e.g. my personal local ruleset basically starts with: <rule ref="WordPress">
<exclude name="WordPress.VIP" />
...
</rule> |
|
@tfrommen I appreciate you taking the time to write down your thoughts and look forward to you getting more involved 👍 . There is some background behind the
The However, on the one hand the handbook is far from comprehensive, there are a lot of areas which are not covered or only implied. On the other hand, there are industry standards and both PHP as well as WP best practices. Tongue in cheek, I would go as far as saying that
We can only hope that one day WP will, but until there is agreement on what those are - and even FIG can't seem to get agreement on a lot of these things - and the WP Core handbook is adjusted to that point, we will need separate rulesets. |
|
I only have the same experience as @Rarst (see the MultilingualPress links in my comment). However, I also always include only |
And you can do that in your custom ruleset without issue, because the current WPCS rulesets are inclusion based ;-) |
Let me put it tad differently — I don't quite follow which part of discussion covers "exclusion-based" and so how original suggestion is preferable as opposed to it? Does anything about my take on starting with separating |
I don't think Separating out just the style sniffs from If I understand you correctly, you propose something along the lines of:
The sniffs used for the Core code style come both from WPCS as well as the PHPCS native upstream standards. So if you are thinking that you could then do the below, you are mistaken as that would not exclude anything in practice. <rule ref="WordPress-Core">
<exclude name="WordPress-Core-Style"/>
</rule>Excluding does not work for compound standards, only for actual sniff standards. Exclusions like the below do work, but you would then need to manually exclude everything which is contained in <rule ref="WordPress-Core">
<exclude name="Squiz"/>
<exclude name="WordPress.Whitespace"/>
</rule> |
That's also in my boilerplate as well. Version numberingWe already do follow semantic versioning, and I'd consider 1.0.0 to be suitable once 41057 gets merged, as a way of ratifying everything that's been done to date. Proposal 1 👎While I like the intent, I do find the increase in rulesets that we'd be providing to be excessive. I appreciate we want adoption outside of core, but I think we're already seeing that anyway (even WP-CLI just had a big update on using code standards) and don't think a re-organisation to make it more complex serves the purpose of increased adoption. For those that want best practices, and not code style, or want checks for new PHP syntaxes, or not, or cheese instead of ham (basically, something more than What we have now is already simple and easy to follow - what the handbook says for code, what the handbook says for docs, what we consider to be best practice, and what VIP requires. I don't feel there is sufficient value for the masses in the re-organisation, that those who do want subsets can't already apply themselves. More exemplar rulesets on the wiki ("How to follow best practices and ignore whitespace checks") would be more than sufficient, than actual PHPCS rulesets that we'd need to maintain. Proposal 2 👍Despite the BC-break (and another reason we've not yet gone to 1.0.0), I'd be in favour of this. I love the idea of making some of the As per my boilerplate, I'm all in favour of just using |
Proposal 1: changing the rulesetsLike I have mentioned before I support the separation of coding styles and best practices. I would like to see the I do not fully understand all of the sniffs in the "Modern" ruleset but I suppose the sniffs are dependant on the version of PHP that is being supported in the project and code features that are going to used. Could we make sniffs detect the constraints or could could we define the rulesets based on the PHP versions? Proposal 2: Moving some sniffs to other categoriesThis gets a 👍 from for the most part. I would like to see I think there is place for a sniff that checks specific function calls if they are being escaped. |
Are you sure on this? I've just tried the following (note <rule ref="WordPress">
<exclude name="WordPress-VIP" />
</rule> |
This was definitely my thinking before. Am I correct in thinking that most of the proposed rulesets could in theory be maintained in a separate project? In other words, if people actually see the value in this, they could maintain it separately, couldn't they? That would keep WPCS simple, but we could still link to such standards for those who want different options. Any of us who want to help maintain it could do so, but perhaps as @GaryJones says it is not the prerogative of WPCS itself and we should not be adding this complexity here in WPCS directly. I guess this comes back to what I had said when this was proposed some time ago: I'm willing to contemplate splitting the standards on WPCS's part, but we need to consider whether it is something that WPCS really needs to do itself or not. What are the drawbacks of trying to do this independent from WPCS, and are they significant enough, and the need for sub-standards great enough, to justify the additional complexity here. And I'm still not clear on that: are there any technical drawbacks to maintaining these standards independently (other than the obvious need for coordinated maintenance)? Maybe it would be better to create a separate repo with the substandards in it, or encourage the community to do so if they think this is helpful, and see whether there is actually enough interest in it/usefulness to make it worth splitting things here. ?? |
|
Yes, this can be done by individuals in their own ruleset, but they would have to amend their ruleset (nearly) every single time we add a new sniff to a ruleset. Over the last year, I have taken note of what people have told me at conferences, meetups, twitter, what I see happening for theme/plugin reviews etc. Am I willing to create a separate repo to maintain a separate set of rulesets shadowing what WPCS does ? Hell no, I'm doing enough as it is, thank you very much. If this is done within WPCS, this is a one-time change and the maintenance burden on us after the change is no different than it is now. If this is done in custom rulesets for individual projects and/or in a separate repo, it creates an extra maintenance burden for every single person maintaining such a custom ruleset/repo which would quickly be a lot more person-hours than the one-time change to WPCS would involve. More than anything, history shows us that projects which shadow other projects very quickly fall into disrepair. That would in effect mean that people would be missing out on new sniffs we're creating and adding and will not be getting the benefits these sniffs bring, which IMHO would be a shame. |
I really appreciate your insight @jrfnl (as it should be fairly obvious by now that I really don't know how other people use these standards 😄). I think that a part of the issue here is that we are trying to address several different interests all at once. Perhaps it would help to break it down a bit, to make discussion easier:
Although we need to take all of these into account in the end, and they do touch at the edges, it is complex to try to think about them all at once. So maybe we could respond to each of these points specifically, rather than the whole proposal in general. (Some of the discussion has already been specifically about each of these aspects, of course, but not always real clear.) I'm trying to get me latest thoughts together now, and I'll leave a comment addressing each of these after I've considered things a bit more. |
I guess the analogy here would be PSR-1 vs PSR-2. I know for a while there were some developers who were happy to follow PSR-1, but didn't see the benefit in PSR-2 (Laravel was a big one), but most of them now follow PSR-2 as well (including Laravel). I'm sure the split between changes with value, and changes for the sake of pretty and consistent code, helped with their migration, but ultimately, they all took on PSR-2 anyway. As such, though folks may be saying in person their wish to just have one part of the standard, ultimately, the WordPress standard, as defined in the handbook, incorporates code style too, i.e. there's less value in the long term to split it out (and folks can do that themselves, or from exemplar wiki guidance, if they really do want to break it down).
Do we make it clear what the "Extra" ruleset is all about? That's about all that is needed here, since it's otherwise self-explanatory (beyond emphasising that the "Core" handbook is really what plugin and theme authors should be using as well i.e. standard for the community.
I'd like to think that the sniffs added for "modern" code generally have no effect when the modern syntaxes are not found, so making a distinction has less value. There's no definition of "modern" beyond being something later than the minimum version of PHP that WP has - not all of which is modern anyway. |
@Rarst Interesting. I'm going to have to run some tests myself, but it may well be that this has changed with PHPCS 3.x in that case, as I'm pretty sure that didn't work in PHPCS 2.x. Thanks for testing. |
|
The original Proposal 1 is based on thinking towards the future and offering maximum flexibility to people creating custom rulesets. Taking the feedback I've received so far into account (and thank you all for joining the discussion, I really do appreciate it!), let me propose the following more minimalistic alternative which would allow for the first and the third usecase and only to a lesser extent for the second: Proposal 3 (alternative to Proposal 1): changing the rulesetsAdding 3 new (subset) rulesets:
The existing rulesets would not change in functionality, but would now all be (slightly less) compound rulesets:
I would like to also propose deprecating the Usecases 1 and 3 can now be addressed as people can chose to include the As for "Modern" code: this would for now mean that only non-blocking modern code sniffs would be accepted and only for |
|
Ok, so PR #1242 has been merged in the mean time. Has anyone had anymore bright ideas on how to move forward regarding Proposal 1 ? |
|
I would still like to see something like Proposal 1 go into this release. I am very flexible as far as naming goes, although since we're renaming things already it would be nice to get the ruleset naming just right. Though we've dropped consideration of Modern for now, we are still dealing with two different concerns, formatting/non-formatting and handbook/non-handbook. (And with VIP thrown into the mix for good measure.) I think this is making it difficult to pinpoint a solution, because the discussion regarding the two different aspects is so easily confused. In the interest of moving forward, I propose that we first focus on the naming regarding handbook/non-handbook. This seems logical, because we already have this distinction in our rulesets now (Core/Extra). Then, after that is settled, we can move on to introducing something new (the distinction between formatting/non-formatting). While making formatting/non-formatting split is the ultimate purpose of Proposal 1, and offers the most benefit, it may need to be postponed to a later release until the naming is figured out. If it is done with sub-rulesets then it will not break backward compatibility, so it doesn't have to go in with the current naming changes. (Of course, if we can get the naming issue resolved, then it can go into this release.) In addition, we want to ultimately get rid of the VIP ruleset, and have that be maintained entirely by Automattic. We need to deprecate it in preparation for that. Any changes (particularly naming ones) related to its deprecation should accompany this release as well, IMO. 👉 So what I am suggesting is that our focus right now would be on settling naming/handling for handbook vs non-handbook things, and deprecating VIP. Getting that out of the way might make it easier to just focus on naming around formatting/non-formatting sub-rulesets. I'd say new issues should be opened for each of these, with a summary of the discussion so far. This issue has become difficult to follow. If nobody has any objections, I can create the issues (some likely exist for VIP already anyway), and we can go from there. 🕐 As far as timing goes, I think it is worth noting that at this time of year people often have less time/attention to devote to things like this. As such, I think a breaking release like this over the next few weeks would be out of the question anyway. So I do not see a need to rush to get this out yet. I don't see the next release happening until sometime after mid-January. |
|
I was trying to think of pairs of synonyms for handbook vs opinionated non-handbook, but a more controversial idea hit - it may not be viable, but may as well mention it. Although the documented coding standards are in the Core handbook, it does explicitly say that it's for the whole WP space (a great thing):
Right now, So here's the suggestion:
It would take some effort on the part of the key people to agree that the parts are Handbook-worthy, and anything that isn't may still be left in some (Aside: I'd like to see WordPress-Docs ultimately be merged in as well - folks could exclude The benefit is a more unified appearance to those who want to follow the WP coding standards. For those who already are, it gives them new opportunities to tighten up their code. For the maintainers here, it's a bit of short-term pain, but should make things simpler in the longer term. To summarise, for
But if VIP was passed on to Automattic, and
Wouldn't that be a nice goal to aim for? |
|
I also mentioned the idea to have dedicated rulesets for Core and plugins/themes. But in a different way than @GaryJones did. As a plugin developer, I don't like all of what So, one could go one of three routes:
Just thinking out loud here... |
This puts the emphasis back on plugins and themes being somewhat secondary to WP Core. WP Core is just yet another piece of software that uses the WordPress [Community] Coding Standards. |
Regarding @JDGrimes' remark about the timing:I agree, there is no rush, but it would be good to keep this ticket (or the sub-tickets) moving. As the next release will now be called Regarding @GaryJones' approach to simplify and consolidate:While I very much applaud and encourage the efforts to move individual rules from
Also, while the handbook claims the rules are for "all WordPress projects", IMO this is just not true. The "community" has AFAICS never had a say in what rules there should be and most of what's there is - again AFAICS - pragmatically based on "what's currently the prevailing trend in Core", which is the wrong way around for standard-design. Regarding merging
|
I'd like to see a release before then, I'd like to update the handbooks and get various other changes into core without having to have committers and contributors shuffle around composer configs by having to install If this involves shipping a p.s. There are various committers and contributors waiting for me/us for the above |
@ntwb Could you elaborate on this ? What changes to the handbook do you want to make ? And why would this have to wait for the release ? WPCS - at least for the If this is about the doc-alignment sniff, that would not be in a |
|
Sorry, not any of the coding standard handbook changes, instructions to install PHPCS, WPCS, adding docs on the new (yet to be committed) Pages such as: There are committers and contributors patiently, eagerly even, waiting for this documentation to be updated and the Grunt tasks to be committed so that PHPCS can be included in their workflow. Currently, committers have no way to test the code they are committing, adding in these Grunt tasks will aid that and save on code churn. The install and configuration of the above needs to be as easy and stable as possible and avoiding moving targets such as having to use WPCS |
|
Here's an example of what I'm wanting to avoid: https://make.wordpress.org/core/2017/11/30/wordpress-php-now-mostly-conforms-to-wordpress-coding-standards/#comment-33334 |
|
@ntwb I totally get where you are coming from, but as I've been conference hopping for the past month and a half, I was already glad to get the principle of the sniff ready for the core patch. It'll need some more work + a sister-sniff before it can be pulled here and that's independent of the discussion in this issue. |
|
Noting here that it's VIPs intention on the longterm to move the VIP ruleset out of WPCS and into our own repository. It's confusing that we have a ruleset that depends on the wpcs VIP ruleset, but we don't use the WPCS VIP ruleset directly |
Just noting for readers, that this was done - WPCS deprecated the VIP-category sniffs in WPCS 1.0, and removed them completely with WPCS 2.0. |
That's great! Then both of the above proposals are out of date and will need to be either dropped or redone:
|
Proposal 2 was already implemented ahead of removing the VIP standard nearly two years ago and released in WPCS 1.0.0.
That has always been the case, that's why there is the As there is no movement on proposal 1, I'm closing this issue now. |
As a follow up on issues #676, #740, #1067, #1156, I would like to propose a restructuring.
I'm opening this as a separate issue as I would like to get as many eyes as possible on this proposal and would like a (preferably unanimous) 👍 / 👎 / 👀 (= seen it, no opinion either way) vote from all the @WordPress-Coding-Standards/wpcs-admins on whether to go ahead with this. /cc @grappler @david-binda
Please vote separately for proposal 1 and proposal 2.
TL;DR
Proposal to split the rulesets up into additional separate rulesets for codestyle and best practices and move some of the sniffs around to make adoption of the best practice sniffs from WPCS easier for the wider WP community.
Justification
While WPCS is based on the Core handbook and originally was probably primarily intended for usage by Core, the WordPress infrastructure is a lot wider than just Core and the way the current rulesets are set up are hindering adoption of WPCS in the wider WordPress community.
Example usecases
Proposal 1: changing the rulesets
Adding 7 new (subset) rulesets:
WordPress-Core-StyleWordPress-Extra-StyleSquiz.Scope.MethodScope,Generic.PHP.SyntaxandWordPress.Classes.ClassInstantiationWordPress-Modern-StylePSR2.Namespaces.NamespaceDeclaration, but also the ReturnTypeSniff from PR #1084 and theusedeclaration sniffs I've been working on to fix #1071WordPress-Core-Best-PracticesWordPress-Extra-Best-PracticesExtraWordPress-Modern-Best-PracticesWordPress.PHP.DiscourageGotoand sniffs checking for untestable code like theGeneric.Metrics.NestingLevelandGeneric.Metric.CyclomaticComplexitysniffsWordPress-VIP-RequirementsVIP, except for the inclusion ofWordPress-CoreThe existing rulesets would not change in functionality, but would now all be compound rulesets:
WordPress-Core=WordPress-Core-Style+WordPress-Core-Best-PracticesWordPress-Extra=WordPress-Core+WordPress-Extra-Style+WordPress-Extra-Best-PracticesWordPress-VIP=WordPress-Core+WordPress-VIP-RequirementsAdditionally an extra compound ruleset would be introduced:
WordPress-Extra-Modern=WordPress-Extra+WordPress-Modern-Style+WordPress-Modern-Best-PracticesAs far as I can see at this moment, the changes would not impact the
WordPress-Docsruleset.Impact analysis
Extraand moved toModern.WordPress-ExtratoWordPress-Extra-ModernPros
It will make maintenance of custom rulesets based on WPCS far more easy and more future-proof.
Cons
The downside of this change is that the amount of rulesets WPCS provides increases exponentially and this could be confusing to new users.
Extra attention will need to be paid to rewriting the Readme section explaining which rulesets are available and which should be used when.
Proposal 2: Moving some sniffs to other categories
There are a number of sniffs where the current category is historically grown, but a different category would be more intuitive and make including whole groups of sniffs easier.
Impact analysis
This would be a breaking change for people including/excluding any of the affected sniffs in custom rulesets.
However, the break could be minimized by deprecating the old sniffs (with notification) and letting these extend the new sniffs for a limited period.
Pros
It will make it easier to include a whole category from a custom ruleset, rather than having to include individual sniffs.
Cons
It will be a BC break.
Planning
I would like to propose releasing WPCS 0.14.0 around the end of this month and having a dedicated 0.15.0 release soon after containing only those PRs necessary for the above proposed changes.
For those who've read this far: thank you for your attention and dedication to WPCS.
Opinions & feedback welcome!
The text was updated successfully, but these errors were encountered: