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

Add PHPCS linting #24

Merged
merged 12 commits into from
Jul 19, 2019
Merged

Add PHPCS linting #24

merged 12 commits into from
Jul 19, 2019

Conversation

johnwatkins0
Copy link
Member

Description of the Change

This makes the following updates to resolve #3 :

  • Installs the 10up PHPCS package with Composer
  • Adds the PHPCS XML config file
  • Adds lint and lint-fix composer scripts
  • Creates package.json file and installs husky and lint-staged dev dependencies
  • Sets up Husky pre-commit hook to run PHPCS before commits.

I also ran PHPCS against the project once. There was only a handful of issues -- all in post-meta.php -- so I fixed them. I also removed a few input var ok comments that aren't needed for the standard we're using.

Potential conflict

Merge conflicts around Composer may need to be worked out between this and #23.

@johnwatkins0 johnwatkins0 changed the title Feature/phpcs linting Add PHPCS linting Jul 12, 2019
@jeffpaul jeffpaul added the type:enhancement New feature or request. label Jul 12, 2019
@jeffpaul jeffpaul added this to the 1.0.0 milestone Jul 12, 2019
.gitignore Outdated
.DS_Store
node_modules
vendor/*
!vendor/composer

Choose a reason for hiding this comment

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

possibly we could ignore these and include them later in a build step

@@ -70,21 +70,26 @@ function enqueue_scripts( $hook ) {
}

// Enqueue the styles.
wp_enqueue_style( 'admin_tenup-auto-tweet', TUAT_URL . '/assets/css/admin-auto_tweet.css', TUAT_VERSION );
wp_enqueue_style( 'admin_tenup-auto-tweet', TUAT_URL . '/assets/css/admin-auto_tweet.css', [], TUAT_VERSION );

Choose a reason for hiding this comment

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

nice catch!

TUAT_VERSION,
true
);

$post_id = get_the_ID();
if ( empty( $post_id ) ) {
$post_id = filter_input( INPUT_GET, 'post', FILTER_VALIDATE_INT );

Choose a reason for hiding this comment

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

is this change for linting purposes? previously we cast the input to an int, so a string of '1' would get cast to 1. Will FILTER_VALIDATE_INT reject non ints? are we sure these values aren't arriving here as strings?

TUAT_VERSION,
true
);

$post_id = get_the_ID();
if ( empty( $post_id ) ) {
$post_id = filter_input( INPUT_GET, 'post', FILTER_VALIDATE_INT );

Choose a reason for hiding this comment

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

is this change for linting purposes? previously we cast the input to an int, so a string of '1' would get cast to 1. Will FILTER_VALIDATE_INT reject non ints? are we sure these values aren't arriving here as strings?

@adamsilverstein adamsilverstein self-requested a review July 16, 2019 20:08
Copy link

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

Let's .gitignore the entire vendor directory so we can avoid changes there.

We don't need the plugin to run without a composer install from master. We will add a build process to bundle up the code required for a release, including the correct vendor directories.

@johnwatkins0
Copy link
Member Author

Thanks, @adamsilverstein! As with the other PR, I've updated to ignore the entire vendor directory.

Regarding your filter_input question, my change was in response to a PHPCS warning about how we should use phpcs:ignore [rule] comments instead of input var ok. But updating how that input was actually processed was out of scope for this issue, so I've taken that down a notch and instead just updated to the approved phpcs:ignore comment format.

Thanks for your feedback! Let me know if you think this is ready to merge. There might be a conflict with the PHPUnit PR for me to resolve

Copy link

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

Nice work!

@johnwatkins0 johnwatkins0 merged commit 5a38802 into master Jul 19, 2019
@johnwatkins0 johnwatkins0 deleted the feature/phpcs-linting branch July 19, 2019 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add phpcs linting [2 hrs.]
3 participants