search

wp-cli / scaffold-command

Generates code for post types, taxonomies, blocks, plugins, child themes, etc.
MIT License
165 stars 87 forks source link

Update PHPCS default rule set #161

Closed NicktheGeek closed 6 years ago

NicktheGeek commented 6 years ago

The .phpcs.xml.dist rule file is detected and implemented automatically, reducing complexity for running phpcs.

The updated rules account for new wpcs sniff properties including the ability to check global prefix and i18n implementation with the correct text domain when specified.

The values have been left blank so the sniffs are generic, but the rules are in place to make it easier to get started by scaffold.

This is tangentially related to https://github.com/wp-cli/scaffold-command/issues/109

This is also loosely related to https://github.com/wp-cli/scaffold-command/issues/63 as updating composer scaffolds could work with this change.

@GaryJones may be able to provide additional feedback on this file change.

schlessera commented 6 years ago

Yes, I would indeed love feedback from @GaryJones and @jrfnl to know whether this would be a good default starting point for new plugins.

schlessera commented 6 years ago

@NicktheGeek For the blanks, it would be helpful to add comments so that developers immediately know where to turn the dials.

NicktheGeek commented 6 years ago

@schlessera good idea.

NicktheGeek commented 6 years ago

@schlessera added the comments.

GaryJones commented 6 years ago

Rushing the review, and @jrfnl can fill in some blanks (or I can another day), but overall +1 for the PR to make it a much nicer starting point to implementing WPCS.

Thanks @NicktheGeek!

NicktheGeek commented 6 years ago

thanks you @GaryJones after you helped me get this on GFWA I've used it on several projects and I can't tell you the number of times it has found a minor typo in the text domain and saved me so much headache down the road. That one feature is worth the price of admission.

NicktheGeek commented 6 years ago

@jrfnl Thanks! I've added the changes you recommended.

@schlessera this may need some additional feedback from @jrfnl or @GaryJones but so far I've implemented almost everything suggested. There are a couple of comments regarding WPCS 1.0.0 but IMO we are better if this is implemented now and an issue created to update when 1.0.0 comes out with additional improvements. I'm good either way though pending release timeline for WP CLI 2.0 and WPCS 1.0.0.

jrfnl commented 6 years ago

Forgot to respond to this:

There are a couple of comments regarding WPCS 1.0.0 but IMO we are better if this is implemented now and an issue created to update when 1.0.0 comes out with additional improvements.

WPCS 1.0.0 was due to come out last week, but there's still one issue which needs to be fixed. Expect it very soon though.

All the same, for the ruleset as it is, AFAICS, no changes would be needed for WPCS 1.0.0, so you're good.

GaryJones commented 6 years ago

The failure looks like some behat tests need updating to reference .phpcs.xml.dist (leading dot) instead of phpcs.xml.dist.

NicktheGeek commented 6 years ago

@GaryJones @jrfnl will it be ok for this to not have the leading .? I think it is required to be automatically used but I want to make sure I'm understanding the reason.

jrfnl commented 6 years ago

@GaryJones @jrfnl will it be ok for this to not have the leading .? I think it is required to be automatically used but I want to make sure I'm understanding the reason.

Do you mean for the actual file name phpcs.xml.dist ? (in contrast to my remark about the exclude pattern https://github.com/wp-cli/scaffold-command/pull/161#discussion_r204142662 )

Yes, no problem.

Allowing a leading . for the filename was introduced in PHPCS 3.1.0 with a file loading order change in 3.1.1.

So for the custom ruleset with a leading dot to work, the minimum PHPCS requirement would have to be set at 3.1.0.

The file loading order (as of PHPCS 3.1.1) is:

  1. .phpcs.xml
  2. phpcs.xml
  3. .phpcs.xml.dist
  4. phpcs.xml.dist

In other words, not having the leading . now allows three different files to overrule the file.

Refs:

GaryJones commented 6 years ago

will it be ok for this to not have the leading .?

And beyond what @jrfnl just said, whether it's right for PHPCS 3.1[.1] to be the minimum version for scaffolded plugins is a different matter, though for new plugins I don't see much reason why the default shouldn't be 3.3.

NicktheGeek commented 6 years ago

I went ahed and updated the tests to solve for the file ext change

jrfnl commented 6 years ago

FYI: in the mean time, both PHPCS 3.3.1 as well as WPCS 1.0.0 have been released.

schlessera commented 6 years ago

Thanks to @NicktheGeek for the pull request, and thanks to @GaryJones & @jrfnl for the extensive review!