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.

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

Allow standard to be selected in composer.json #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NickDickinsonWilde
Copy link

No description provided.

Copy link
Member

@schlessera schlessera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @NickWilde1990, thanks for the PR!

I did a review and have a few nitpicks. But I agree with the approach and would like to merge this once we can take care of the few change requests.

@@ -1 +1,2 @@
/vendor/
/.idea/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ignore should go into your global gitignore list, as it is not a direct artifact of the project, but rather a detail of your personal environment.

WPCS provides 5 different coding standards (`WordPress-VIP`, `WordPress`,
`WordPress-Extra`, `WordPress-Docs` and `WordPress-Core`). By default this uses
`WordPress-Extra`. If you want to use another one of the standards, you can
specify it in your project's composer.json's `extra` key:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reorder for readability

Suggested change
specify it in your project's composer.json's `extra` key:
specify it in the `extra` key of your project's `composer.json` file:

@@ -39,9 +39,15 @@ public function preCommit()

echo 'Running PHP CodeSniffer in ' . $this->root . PHP_EOL;
$sniffer = new PHP_CodeSniffer_CLI();
@$config = $this->getExtraKey('php-composter-phpcs-wpcs', [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid the "shut-up" operator (@ ). The getExtraKey() already verifies that the key does indeed exist and falls back to the value you provide if not. Any other error should either be dealt with separately, or bubble up because it might be pointing to a bigger issue.

Suggested change
@$config = $this->getExtraKey('php-composter-phpcs-wpcs', [
$config = $this->getExtraKey('php-composter-phpcs-wpcs', [


ob_start();
$numErrors = $sniffer->process(array('standard' => 'WordPress-Extra', 'files' => $files));
$numErrors = $sniffer->process([
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you think about merging the $files into the $config array here? This way, one could even pass other PHPCS/WPCS settings through the configuration in composer.json. Not sure there's much that makes sense, though...

@schlessera schlessera requested a review from javorszky January 23, 2019 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants