Require blt validate:phpcbf

[jayakrishnanj]

Its not a bug, requesting an enhancement.

Now we have blt validate:phpcs for validating, which we can use locally to validate the Code. Similarly if we have blt validate:phpcbf to fix the CS issue, it would help us all.

[grasmash]

Yeah I agree, I've wanted to create this feature for some time.

[dooleymatt]

@grasmash, @jayakrishnanj I'd be happy to take this on if you'd like.

[grasmash]

@dooleymatt Yes please!

[TravisCarden]

I, too, would like to see this command. @dooleymatt, I merely request that 1) it doesn't go under validate because it's not merely a validator--it's a fixer. And 2) for the same reason, it doesn't run automatically. Some of us would riot in the streets if our pre-commit hook one day started changing our code without asking. :)

[dooleymatt]

@TravisCarden sounds good. Thanks for the input.

I've been struggling to figure out which files should be fixed with this function. So far, my thinking has been that the most common use of this function is probably going to be to fix files that are flagged with PHPCS violations in the pre-commit hook. Which, to me means that the files that are fixed should probably default to the files that are staged for commit. I agree that it shouldn't run automatically, however, I feel like there should either be an interactive prompt to the user asking if they want to automatically fix violations when they are found, or a message that just says run blt phpcbf to fix the violations.

Additionally, I think that there should be PHPCBF tasks that would mirror the PHPCS tasks. So, one that would fix all custom code (with the same filters as are used in the PHPCS task), one that would fix a specific fileset, and one that would fix a single file. But, I feel like the default behavior should be to only fix staged files.

@TravisCarden, @grasmash, @jayakrishnanj do you guys agree, or do you have any other thoughts on this?

[TravisCarden]

In the spirit of the Unix philosophy, I would argue for the primary use case being to run on an arbitrary list of files and directories provided by command line argument. That way it has the most generic interface possible, which supports both developer use and piping out-of-the-box. From there it would be easy to write integrations that, for example, pipe in a list of uncommitted files on a given hook. If you wanted to, you could default the scope if no arguments are provided, and cover more or less all your bases.

[dooleymatt]

you could default the scope if no arguments are provided

@TravisCarden I think I agree completely, but, could you explain what you mean by this? Are you saying that the files argument should default to the list of staged files?

Also, if the command is not validate:phpcbf, what do you think it should be?

[TravisCarden]

@dooleymatt I incline to say the default behavior (given no command line arguments) should be to test not just changed or staged files but all custom directories. On CI, that's what you would want to do, and as a TA I can imagine good use cases for it. You could add one or more flags like --changed-only or --staged-only for use in the pre-commit hook, if you wanted to take that complexity from client code.

I would suggest a new fix category for the command: i.e., fix:phpcbf. None of the current categories fit, and I can see adding other "fixers" in the future.

[dooleymatt]

Dig it! Thanks @TravisCarden!

[grasmash]

Merged in #1019