Support ruleset as argument #6
Conversation
- Add support for passing ruleset as option/argument Facilitates working with custom ruleset / experiment on rules. - Add support for passing branches (base and current) as arguments Now it's feasible to run php-cs / php-cs-diff against unstaged changes (neat) and customize branching options. - Added helper methods to handle flags, options and arguments - Remove usage of ngettext as extension may not be present and it's overkill
| $lines = array_filter(explode(PHP_EOL, $lineDiff)); | ||
| $command = 'git diff -U0 ' . $this->baseBranch . ' ' . $this->currentBranch . ' ' . $file; | ||
| $output = shell_exec($command); | ||
| preg_match_all($pattern, $output, $lineDiff); |
There was a problem hiding this comment.
The grep command was used originally (within the shell_exec()) to reduce the memory footprint for PHP. I would be concerned that running this on a very large diff would create an unnecessary memory footprint.
However, I do agree that this feels like a cleaner approach, but I would prioritise functionality before clean code. If that helps explain why it was done in the first place, do you think a comment to explain why would suffice?
There was a problem hiding this comment.
I understand. The reason I replaced the current implementation is due to the differences in flags between grep on OSX/bsd and GNU. Running it on OSX gave an error.
Maybe we can work on providing strategies that can be configured (ie, use grep, use preg_match)?
There was a problem hiding this comment.
Yes, that sounds like a great idea. That, or we can automatically detect the environment to decide on the method or command to use.
|
|
||
| $this->baseBranch = 'origin/' . str_replace('origin/', '', $this->argv[1]); | ||
| $this->currentBranch = trim(shell_exec('git rev-parse --verify HEAD')); | ||
| protected function getArgument($index, $default = null) |
There was a problem hiding this comment.
Please could we add some docblocks to keep the class consistent with the other methods?
| return isset($arguments[$index]) ? $arguments[$index] : $default; | ||
| } | ||
|
|
||
| protected function getOption($name, $default = null) |
There was a problem hiding this comment.
Again, some dockblocks would help keep the class consistent.
|
|
||
| if ($this->isVerbose) { | ||
| if (!$this->baseBranch && !$this->currentBranch) { | ||
| $this->climate->comment("Comparing unstaged changes."); |
| foreach ($this->argv as $arg) { | ||
| list($flag, $val) = explode('=', $arg); | ||
|
|
||
| if ($flag == $name) { |
There was a problem hiding this comment.
Please use strict-type checking
| if ($flag == $name) { | |
| if ($flag === $name) { |
| protected function getArgument($index, $default = null) | ||
| { | ||
| $arguments = array_filter($this->argv, function ($val) { | ||
| return strpos($val, '-') === false; |
There was a problem hiding this comment.
Could we change this to use yoda-style conditions?
| return strpos($val, '-') === false; | |
| return false === strpos($val, '-'); |
|
|
||
| Basic example | ||
|
|
||
| ```shell | ||
| phpcs-diff develop -v | ||
| phpcs-diff -v origin/develop develop |
There was a problem hiding this comment.
I would rather this option be added as a feature, rather than a breaking change to existing functionality.
With your changes, we will have 3 variations:
phpcs-diff- Run a diff against unstaged changes.phpcs-diff origin/master- Run a diff against the base branch (defined) and the current branch (determined bygit rev-parse --verify HEAD)phpcs-diff origin/ master master- Run a diff against the base branch and current branch (both defined)
My only ask here is to keep the functionality of 1.
| @@ -133,12 +147,11 @@ public function run() | |||
| if ($this->isVerbose) { | |||
| $fileDiffCount = count($fileDiff); | |||
| $this->climate->comment( | |||
| 'Checking ' . $fileDiffCount . ' ' . | |||
| ngettext('file', 'files', $fileDiffCount) . ' for violations.' | |||
| 'Checking ' . $fileDiffCount . ' file(s) for violations.' | |||
There was a problem hiding this comment.
This should be able to fit on the same line now.
|
Thanks for the comments. I'll address them shortly 👍 |
Added support for ruleset option and defining branches
Facilitates working with custom ruleset / experiment on rules.
Now it's feasible to run php-cs / php-cs-diff against unstaged changes (neat) and customize branching options.
Added helper methods to handle flags, options and arguments
Remove usage of ngettext as extension may not be present and it's overkill
Remove usage of grep binary and use preg_* extension instead