Restrict hook to the changed code

[rodrigoprimo]

Would it be possible to modify the hook to check only the code that was added in a given commit (instead of checking changed files as in #1)? I think this would reduce friction even more when using wp-enforcer into an existing codebase.

[stevegrunwell]

I agree, it would certainly reduce friction, but I'm not sure it will be possible given the way PHP_CodeSniffer tokenizes files; perhaps filtering the output from phpcs to remove any lines outside the given changeset?

[rodrigoprimo]

I'm sorry but I'm not familiar with PHP_CodeSniffer so I'm afraid I'm unable to help.

If I'm not mistaken, @westonruter mentioned something like this a few years ago in a conversation during WordCamp San Francisco. But I don't remember if he was just throwing an idea or if he had implemented something.

[stevegrunwell]

Not a problem, drawing attention to an issue is helping all in of itself!

I wouldn't be surprised if Weston mentioned the coding standards, considering he's one of the maintainers of the WordPress Coding Standards sniffs for PHP_CodeSniffer ;)

[westonruter]

Hey guys. What is described here is exactly what I implemented in wp-dev-lib for Travis CI checks, to restrict error reporting for PHPCS, JSHint, and JSCS for only the lines that are changed in a given pull request.

For more info, see https://github.com/xwp/wp-dev-lib/#limiting-scope-of-travis-ci-checks

I've also implemented the same for the WordPress Core project in a patch: https://core.trac.wordpress.org/ticket/34694#comment:6

I have an outstanding todo item to implement the patch-checking functionality in pre-commit hook (in addition to the Travis CI context): xwp/wp-dev-lib#100

[ghost]

Would it be possible to modify the hook to check only the code that was added in a given commit (instead of checking changed files as in #1)?

Not a good idea. Checking only what changed adds complexity to the project and hiders the usefulness of catching linters early as committing that code will only end up in them running and getting flagged in CI, slowing the development process.

Please consider closing this issue.

[westonruter]

I disagree. The wp-dev-lib project by default only reports linting failures on patches. Not only is this checked on Travis CI but also in pre-commit. This I see as essential for introducing linting for an existing project to avoid having to do the massive churn required to make an entire project free of linting issues.

[ghost]

This I see as essential for introducing linting for an existing project to avoid having to do the massive churn required to make an entire project free of linting issues.

If an existing project were introducing linting wouldn't they just roll their own tool anyway? Thinking about the Pareto principle and the scope and size of a package like this it seems awkward at best to think it could accommodate everyone's needs. And if I were running a project in production I'd be very leery of banking on a dev dependency which hasn't seen a 1.0.0 anyway.

I'd imagine such existing codebases would want to go file by file assuming and run whatever safe fixes were available so they weren't afraid to refactor their codebases:

https://github.com/squizlabs/PHP_CodeSniffer/wiki/Fixing-Errors-Automatically

[ghost]

Also, for what's it's worth, the current pre-commit hook in master (and develop) already restricts to changes. So I'm not quite sure why this issue is open unless it's for the sake of discussion.

[stevegrunwell]

The restriction currently in place is on a per-file basis; git diff --diff-filter=... will filter the files to compare, but PHP_CodeSniffer could still pick up coding standards issues in code that hasn't been touched.

For a practical example, consider the following file:

<article>
  <h1><?php the_title(); ?></h1>
  <?php the_content(); ?>
</article>

<div class="sidebar">
  <p><?php _e( 'This is some unescaped text', 'text-domain' ); ?></p>
</div>

Now, consider I make the following change to leverage the_title()'s arguments:

- <h1><?php the_title(); ?></h1>
+ <?php the_title( '<h1>', '</h1>' ); ?>

When I try to commit this file, WP Enforcer will get upset because of the unescaped _e() in the sidebar and prevent me from committing (as that should be esc_html_e()). The goal of this ticket is to restrict the sniffed code not just to files with changes, but to the lines that have changed.

[ghost]

Got it. It's an unusual practice to only lint on diff'd lines. But I'm gathering you may be gearing this tool toward agency use as opposed to common man.

[stevegrunwell]

It's not that it's not designed for the common developer, but WP Enforcer was conceived while I was working at 10up. A lot of code review issues that came up were standards related ("this string needs to be escaped", "this form is missing nonce verification", etc.). A tool like WP Enforcer could be installed as a Composer dependency on a project, ensuring that new code being committed was being checked (via PHP_CodeSniffer) for common coding errors.

Try not to think of WP Enforcer is a strict linting tool, so much as a "catch coding issues before they make it into the repository" tool. As a side effect, PHP_CodeSniffer ends up installed on the project with a ruleset tailored to match the project (per the phpcs.xml file), so developers could lint the entire project via ./vendor/bin/phpcs --standard=phpcs.xml ./.

For a project that's been developed with WP Enforcer from the beginning, restricting analysis to only changed code is rather insignificant (as, in theory, everything written should meet the coding standards); where this issue becomes relevant is when WP Enforcer is added to an existing codebase, eliminating the "grr, WP Enforcer keeps flagging files with pre-existing issues and preventing me from committing my perfectly-valid changes!" pain point (as demonstrated in the example above).

[ghost]

Got it. And it's primary purpose is solid. It helps close the feedback loop for the developer, and saves time and CI cycles waiting for persnickety rule changes to fail the build. If you haven't read the JavaScript standard and why it was created please check it out because they did it right. They do it by the numbers.