Add an interactive mode to phpcbf

[akirk]

Is your feature request related to a problem?

It can be taxing to get larger amounts of code to comply with the coding standards. Be it because you realize you forgot an important ruleset for a project you want to contribute, or you're merging codebases.

What typically happens is that you are confronted with a large amount of violations that can feel overwhelming. Depending on the size of the codebase it can take days to manually go through all the violations and determine how to address them.

Describe the solution you'd like

I think phpcbf should be more supportive with getting a code base to comply and this can be on multiple levels:

• auto-fix more problems
• help me phpcs:ignore that particular line
• assist me in editing the line to make it compliant
• make it easy to decide that the sniff is not for me exclude it in phpcs.xml

In order to achieve this, I propose to extend the interactive mode (-a) that exists in phpcs today to phpcbf.

Some sniffs have auto-fixers, some sniffs seem like they should have auto-fixers but there are multiple ways to fix the problem.

By giving the user a chance to decide how to fix the problem, we can make it faster and less frustrating to get a codebase compliant.

Additional context

Let's say we have a file like this and certain sniffs enabled:

<?php

//helo
class test
{
echo 'hello';
}

then phpcs would complain about it like this:

FILE: test.php
----------------------------------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 4 LINES
----------------------------------------------------------------------------------------------
 1 | ERROR | [ ] You must use "/**" style comments for a file comment
 3 | ERROR | [x] No space found before comment text; expected "// helo" but found "//helo"
 3 | ERROR | [ ] Inline comments must start with a capital letter
 3 | ERROR | [ ] Inline comments must end in full-stops, exclamation marks, or question marks
 4 | ERROR | [ ] Test name must begin with a capital letter
 6 | ERROR | [x] Line indented incorrectly; expected at least 4 spaces, found 0
----------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------------------

As you can see, some violations can be fixed, most cannot. The reason for those is that there is no auto-fixer implemented for them, and often the reason is that there is no one-and-only-way to fix this.

So let’s see how interactive mode would work here. In this first case I’d like to show a situation where there is no auto-fixer available. These would be the options:

PHPCBF INTERACTIVE MODE - test.php
 
ERRORS at line 1, column 6:
  You must use "/**" style comments for a file comment
  Sniff: PEAR.Commenting.FileComment.WrongStyle
  (Not auto-fixable)
 
        1> <?php
        2  
        3  //helo
Choose an action:
  [i] Add a phpcs:ignore to this line (phpcs:ignore)
  [d] Disable this sniff for this file (phpcs:disable)
  [p] Project-wide: exclude this sniff in phpcs.xml
  [e] Edit the file manually
  [s] Skip this violation
  [q] Quit
Action (default: edit):

Now let’s look at a case where an auto-fixer is available. In non-interactive mode, phpcbf would just fix it, but here you have the option to skip it, too. It would be nice if it even could preview the auto-fix:

ERRORS at line 6, column 1:
  Line indented incorrectly; expected at least 4 spaces, found 0
  Sniff: Generic.WhiteSpace.ScopeIndent.Incorrect
  (Auto-fixable)
 
        4  class test
        5  {
        6- echo 'hello';
        6+      echo 'hello';
        7  }
        8  
Choose an action:
  [f] Fix automatically
  [i] Add a phpcs:ignore to this line (phpcs:ignore)
  [d] Disable this sniff for this file (phpcs:disable)
  [p] Project-wide: exclude this sniff in phpcs.xml
  [e] Edit the file manually
  [s] Skip this violation
  [q] Quit
Action (default: auto-fix):

Now let's look an example where it seems like an auto-fixer could exist but it does not because there are multiple ways to fix it. For this one, we'd have an interactive fixer that gives the user the choice how to fix it:

ERRORS at line 3, column 1:
  Inline comments must end in full-stops, exclamation marks, or question marks
  Sniff: Squiz.Commenting.InlineComment.InvalidEndChar
  (Interactive fixes available)
 
Interactive Fix Options:
  [1] Add period (.)
        1  <?php
        2  
        3- // Helo
        3+ // Helo.
        4  class test
        5  {
  [2] Add exclamation mark (!)
        1  <?php
        2  
        3- // Helo
        3+ // Helo!
        4  class test
        5  {
  [3] Add question mark (?)
        1  <?php
        2  
        3- // Helo
        3+ // Helo?
        4  class test
        5  {
  [4] Remove comment
        1  <?php
        2  
        3- // Helo
        4  class test
        5  {
 
Choose an action:
  [1] Apply: Add period (.)
  [2] Apply: Add exclamation mark (!)
  [3] Apply: Add question mark (?)
  [4] Apply: Remove comment
  [i] Add a phpcs:ignore to this line (phpcs:ignore)
  [d] Disable this sniff for this file (phpcs:disable)
  [p] Project-wide: exclude this sniff in phpcs.xml
  [e] Edit the file manually
  [s] Skip this violation
  [q] Quit
Action (default: 1 "Add period (.)"): 

One could argue that you can do all of this today but I think with such an interactive mode we can relieve much of the pain it causes to get a code base to comply by streamlining what you need to do already today anyway: go through each violation and try to fix it.

Some of the choices I haven't described yet:

• i would just append a // phpcs:ignore Sniff.Name to the line
• d would add a // phpcs:disable Sniff.Name
• p would add an <exclude ref="Sniff.Name"/> to your phpcs.xml
• e would open an editor at the line of the code

In summary, this is a proposal to improve the user experience. I think this could transform an experience where phpcs is the tool that scolds you, to the tool that assists you in unifying a codebase to certain standards.

How would this be implemented from the sniff perspective?

Similarly to auto-fixers which use a addFixableError or addFixableWarning, I would propose to introduce a addInteractiveFixableError (and maybeaddInteractiveFixableWarning) method to the File class that the sniff would call.

This addInteractiveFixableError would receive an array of options, each represented by a class of InteractiveOption that describes the fix and has the code to apply it. This would then be used to generate the preview diff and apply the fix if the user chooses to do so.

Down the line, I can see that it makes sense to consolidate this all into addError where the number of provided fixes determines whether it can be auto-fixed (1) or not (0). We will probably need a flag where a single fix cannot be auto-applied but still presented to the user as an option.

Example implementation

Because I believe that something user facing and interactive can be better understood by experiencing it, I have implemented this in my fork according to my vision so that you can check out the code and run this locally.

I've added a few interactive fixes as a demo to Squiz.Commenting.InlineComment and PEAR.NamingConventions.ValidClassName, and have personally used this interactive mode to clean up all the little things that turned up in my branch.

git clone git@github.com:akirk/PHP_CodeSniffer.git
cd PHP_CodeSniffer
git checkout phpcbf-interactive
./bin/phpcbf -a check-me-for-interactive-demo.php

In my implementation example (see below), it worked well for me to "have the option be responsible for fixing the content:" for example, ReplaceOption covers the standard use case of using replaceToken and the fix happens in an applyFix method. The sniff would then call addInteractivelyFixableError with an array of "fix options." This helps separating the sniffing from the fixing, and for other types of fixes, more generic fix options could be created (for example an IndentOption).

How to Implement It in PHP_CodeSniffer

I just want to acknowledge that for implementing this in this code base, this will likely need to split into multiple issues/PRs.

I see the opportunity to split it in this way:

• Activating interactive mode for phpcbf where it stops on each violation and gives you the option to skip or use the auto-fixer if available.
• Implement an interactive fixer with a single implementation of a fix for an error code in a single sniff.
• Single PRs for adding more interactive fixers throughout the sniffs and error codes.
• Add an "edit" option which opens the user's editor to edit the code at the violating position.
• Add an "ignore" option where it adds a // phpcs:ignore comment or adds to an existing comment in the line.
• Add a "disable" option for the sniff in the file (this could be with a php:disable or with an file-specific exclude rule in phpcs.xml (and its variations).
• Add a "project-wide disable" option for the sniff by adding an exclude rule in phpcs.xml (and its variations).
• Consolidate addError, addFixableError, and addInteractivelyFixableError to a single function that takes a $fixOptions array that depending on the number of entries determines the fixability.

Apart for the first two, they could move forward mostly independently from each other, so that controversial discussions on one option don't block other options.

• I have read the Contribution Guidelines and this is not a support question.
• I intend to create a pull request to implement this feature.

[akirk]

I have also blogged about this, the blog post shows the colors of the terminal output.