Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ordering of the rules in the rulesets #602

Closed
jrfnl opened this issue Jul 14, 2016 · 17 comments
Closed

Ordering of the rules in the rulesets #602

jrfnl opened this issue Jul 14, 2016 · 17 comments
Labels
Meta: Repo Type: Question

Comments

@jrfnl
Copy link
Member

jrfnl commented Jul 14, 2016

Each time I'm currently adding a rule, I keep wondering where to put it - beginning of file, end of file, somewhere near a related rule ?

What about re-ordering the core ruleset to follow the sections of the handbook ? That way things have a logical place and it'll be more easy to check what is and isn't covered yet from the handbook.

Opinions ?
@westonruter
Copy link
Member

Agreed.

@GaryJones
Copy link
Member

As good an order as any.

@jrfnl
Copy link
Member Author

jrfnl commented Jul 16, 2016

Re-ordering now and running into some sniffs which are not in the handbook (or at least: I've not been able to find them).
I'm not saying they shouldn't be in the core ruleset - to me it is totally obvious that they should - I'm just wondering if they should maybe be made explicit in the handbook.

  • <rule ref="Generic.PHP.Syntax" /><!-- Check for PHP Parse errors. -->

I'd suggest: While it goes without saying that code shouldn't have parse errors, there is also the thing about supporting multiple PHP versions (5.2-7.1-alpha+HHVM).

So maybe this should actually be moved to extra as often enough there will be code which will only be loaded for higher PHP versions with a wrapper and fall-backs for lower PHP versions.

  • <rule ref="Generic.Files.ByteOrderMark" /><!-- Important to prevent issues with content being sent before headers. -->

I'd suggest: Make this explicit in the handbook.

  • <rule ref="Generic.Files.LineEndings">...</rule><!-- All line endings should be \n. -->

    I'd suggest: Make this explicit in the handbook.

  • <rule ref="Generic.Files.EndFileNewline"/><!-- All files should end with a new line. -->

    I'd suggest: Make this explicit in the handbook.

  • <rule ref="Generic.PHP.LowerCaseConstant"/><!-- Lowercase PHP constants, like true, false and null. -->

    I'd suggest: Make this explicit in the handbook.

  • <rule ref="Generic.PHP.LowerCaseKeyword"/> // Relates to PHP keywords such as 'class', 'function' etc

    I'd suggest: Make this explicit in the handbook.

  • <rule ref="WordPress.WP.I18n"/>

I'd suggest: Make this explicit in the handbook.

Opinions ?

@GaryJones
Copy link
Member

There are lots of things we'd like to see made explicit in the handbook - certainly these are some of the less controversial - but there's no established process from getting buy in from the powers that be.

@jrfnl
Copy link
Member Author

jrfnl commented Jul 16, 2016

the powers that be

  1. And who may that be ?
  2. Isn't the general rule of thumb that anything not covered in the handbook should go in extra ? So - again while I agree with these rules - how come they are in core ?

@grappler
Copy link
Member

@DrewAPicture may be able to help us.

@JDGrimes
Copy link
Contributor

Honestly, if it were up to me I probably wouldn't include that sniff at all. If you are using PHPCS along with other tools (like your IDE) you'll already be checking syntax anyway. This sniff then unnecessarily duplicates that, wasting time and resources. I'd prefer it to at least be moved to extra, but I'll be disabling it in my custom ruleset either way. 😄

the powers that be

And who may that be ?

WordPress core contributors who have write access to the handbook. WPCS has no control over WordPress's standards, and never has. Maybe it's about time that we started coordinating more though...

@jrfnl
Copy link
Member Author

jrfnl commented Jul 16, 2016

Maybe it's about time that we started coordinating more though...

Sounds like a plan 👍

@jrfnl
Copy link
Member Author

jrfnl commented Jul 17, 2016 •
edited
Loading

Ok, nearly done.
PRs #599, #604 and #608 all result from the re-ordering. [Edit] + #612 + #615 +#598
Issue #606 is related to it as well.

The Handbook rule relating to Formatting SQL statements is for the most part covered by the WordPress.WP.PreparedSQL sniff, so I'm now wondering why that sniff is only included in extra and vip and not in core ?

@GaryJones
Copy link
Member

Maybe it's about time that we started coordinating more though

I've tried at least twice to have a separate channel created in the WP Slack, for WPCS discussions (at least to see who has a vested interested in discussing it), as a way to move things forward. I've been told both times that it doesn't warrant it's own channel, and that the main core dev channel should be used instead (despite the fact these standards are meant for a lot more people than core). So feature plugins and other topics get their own channel, to save discussions getting lost amongst everything else, but something which affects (or should affect) every single core, theme and plugin developer, doesn't.

There are so many holes in the Handbook re CS, that even if we did sniff 100% accurately, there would still be a wide range of coding styles. The Handbook simply isn't prescriptive enough to sufficiently cover all the cases in a way that I think it should.

I'm open for ideas from @DrewAPicture and @westonruter about how we can get Core committer buy-in on this to move the Handbook forward (or get new WP commits run against WPCS via Travis i.e. this commit from two days ago would fail the check when it was an easy win to make it pass), so that we've all got something comprehensive to work against.

@westonruter
Copy link
Member

I do have a ticket with a patch that would add PHPCS to Travis CI checks for WordPress Core commits, checking the errors reported among lines changed instead of against the entire codebase: https://core.trac.wordpress.org/ticket/34694 (uses same code as is now in wp-dev-lib). I need to update the patch.

@jrfnl
Copy link
Member Author

jrfnl commented Jul 18, 2016 •
edited
Loading

Work in progress can be found here: 3d501d9 1533053

@jrfnl
Copy link
Member Author

jrfnl commented Jul 22, 2016

Anyone an opinion about my previous question:

The Handbook rule relating to Formatting SQL statements is for the most part covered by the WordPress.WP.PreparedSQL sniff, so I'm now wondering why that sniff is only included in extra and vip and not in core ?

@westonruter
Copy link
Member

@jrfnl not sure. Perhaps the rules weren't part of the handbook at that time.

@DrewAPicture
Copy link
Member

@jrfnl not sure. Perhaps the rules weren't part of the handbook at that time.

This is probably what happened. As I'm sure we're all aware, the handbook isn't codified, it's constantly being refined. So it's very possible that it changed since the sniff was written.

@jrfnl
Copy link
Member Author

jrfnl commented Jul 22, 2016

So shall I move it over to core ? and move the syntax check back to extra ?

@westonruter
Copy link
Member

@jrfnl better to ask for forgiveness than permission 😉

@jrfnl jrfnl mentioned this issue Jul 27, 2016
Closed
jrfnl added a commit that referenced this issue Jul 28, 2016
@jrfnl
Order the core ruleset according to the handbook.
f4b798e 
This PR does not add or remove any new rules, it just re-orders the core ruleset to follow the handbook and documents the rules as they are in the handbook - both covered and not covered.

Closes #602

For any core rules which are not yet covered, but *could* possibly *be* covered, a new issue has been opened if one didn't exist already and the link to the issue is included in the ruleset.

Rules included, but not (explicitly) covered in the handbook are listed in their own section at the bottom of the ruleset.

I've made three changes which should be noted:
* Removed the `WordPress.WP.I18n` sniff from the `extra` ruleset as it is included in the `core` ruleset and `core` is included in `extra`.
* Moved the `WordPress.WP.PreparedSQL` sniff from `extra` to `core` as the checks included in that sniff are actually part of the core rules.
* Moved the `Generic.PHP.Syntax` to `extra` as IRL often enough there will be code which will only be loaded for higher PHP versions with a wrapper and fall-backs for lower PHP versions.

Feedback welcome. Review is probably easiest by just looking at the new version of the file as the diff will be horrible ;-)
@jrfnl jrfnl mentioned this issue Jul 28, 2016
Merged
grappler referenced this issue in WPTT/WPThemeReview Sep 25, 2016
@jrfnl @grappler
Order the core ruleset according to the handbook.
2360919 
This PR does not add or remove any new rules, it just re-orders the core ruleset to follow the handbook and documents the rules as they are in the handbook - both covered and not covered.

Closes #602

For any core rules which are not yet covered, but *could* possibly *be* covered, a new issue has been opened if one didn't exist already and the link to the issue is included in the ruleset.

Rules included, but not (explicitly) covered in the handbook are listed in their own section at the bottom of the ruleset.

I've made three changes which should be noted:
* Removed the `WordPress.WP.I18n` sniff from the `extra` ruleset as it is included in the `core` ruleset and `core` is included in `extra`.
* Moved the `WordPress.WP.PreparedSQL` sniff from `extra` to `core` as the checks included in that sniff are actually part of the core rules.
* Moved the `Generic.PHP.Syntax` to `extra` as IRL often enough there will be code which will only be loaded for higher PHP versions with a wrapper and fall-backs for lower PHP versions.

Feedback welcome. Review is probably easiest by just looking at the new version of the file as the diff will be horrible ;-)
JDGrimes pushed a commit that referenced this issue Oct 8, 2016
@jrfnl @JDGrimes
Order the core ruleset according to the handbook.
62cc26f 
This PR does not add or remove any new rules, it just re-orders the core ruleset to follow the handbook and documents the rules as they are in the handbook - both covered and not covered.

Closes #602

For any core rules which are not yet covered, but *could* possibly *be* covered, a new issue has been opened if one didn't exist already and the link to the issue is included in the ruleset.

Rules included, but not (explicitly) covered in the handbook are listed in their own section at the bottom of the ruleset.

I've made three changes which should be noted:
* Removed the `WordPress.WP.I18n` sniff from the `extra` ruleset as it is included in the `core` ruleset and `core` is included in `extra`.
* Moved the `WordPress.WP.PreparedSQL` sniff from `extra` to `core` as the checks included in that sniff are actually part of the core rules.
* Moved the `Generic.PHP.Syntax` to `extra` as IRL often enough there will be code which will only be loaded for higher PHP versions with a wrapper and fall-backs for lower PHP versions.

Feedback welcome. Review is probably easiest by just looking at the new version of the file as the diff will be horrible ;-)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Meta: Repo Type: Question
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@GaryJones @westonruter @jrfnl @DrewAPicture @grappler @JDGrimes