Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,24 +19,32 @@ This will hopefully put you in a position where your codebase will become more c
phpcs-diff - detect violations based on a git diff

SYNOPSIS
phpcs-diff [BASE_BRANCH]... [OPTION]...
phpcs-diff [-v] [--ruleset=PSR2] [BASE_BRANCH] [CURRENT_BRANCH]

OPTIONS
Here is a (very) short summary of the options available in phpcs-diff.

-v
increase verbosity
--ruleset
defines which php-cs ruleset to use (for example: PSR1, PSR12, ...)

Basic example

```shell
phpcs-diff develop -v
phpcs-diff -v origin/develop develop
Copy link
Owner

Choose a reason for hiding this comment

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

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:

  1. phpcs-diff - Run a diff against unstaged changes.
  2. phpcs-diff origin/master - Run a diff against the base branch (defined) and the current branch (determined by git rev-parse --verify HEAD)
  3. 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.

```

Where the current branch you are on is the branch you are comparing with, and `develop` is the base branch. In this example, `phpcs-diff` would run the following diff statement:
Where `origin/develop` is the base branch comparing with and `develop` is the current branch.

```shell
git diff my-current-branch develop
git diff origin/develop develop
```

You can check current unstaged files with:

```shell
phpcs-diff
```

## Installation
Expand Down Expand Up @@ -67,7 +75,7 @@ You can also download the `phpcs-diff` source and create a symlink to your `/usr
git clone https://github.com/olivertappin/phpcs-diff.git
ln -s phpcs-diff/bin/phpcs-diff /usr/bin/phpcs-diff
cd /var/www/project
phpcs-diff master -v
phpcs-diff -v origin/master master

## Requirements

Expand Down
77 changes: 46 additions & 31 deletions src/PhpcsDiff.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

class PhpcsDiff
{
const DEFAULT_RULESET = 'ruleset.xml';

/**
* @var array
*/
Expand Down Expand Up @@ -55,18 +57,43 @@ public function __construct(array $argv, CLImate $climate)
$this->isVerbose = true;
}

if (!isset($this->argv[1])) {
$this->error('Please provide a <bold>base branch</bold> as the first argument.');
return;
$this->ruleset = $this->getOption('--ruleset', self::DEFAULT_RULESET);

$this->baseBranch = $this->getArgument(0, '');
$this->currentBranch = $this->getArgument(1, '');

if ($this->isVerbose) {
if (!$this->baseBranch && !$this->currentBranch) {
$this->climate->comment("Comparing unstaged changes.");
Copy link
Owner

Choose a reason for hiding this comment

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

Please use single quotes

} else {
$this->climate->comment(
'Comparing branch: "' . $this->baseBranch . '" against: "' . $this->currentBranch . '"'
);
}
}
}

$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)
Copy link
Owner

Choose a reason for hiding this comment

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

Please could we add some docblocks to keep the class consistent with the other methods?

Copy link
Author

Choose a reason for hiding this comment

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

Sure thing!

{
$arguments = array_filter($this->argv, function ($val) {
return strpos($val, '-') === false;
Copy link
Owner

Choose a reason for hiding this comment

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

Could we change this to use yoda-style conditions?

Suggested change
return strpos($val, '-') === false;
return false === strpos($val, '-');

});
$arguments = array_values($arguments);

if (empty($this->currentBranch)) {
$this->error('Unable to get <bold>current</bold> branch.');
return;
return isset($arguments[$index]) ? $arguments[$index] : $default;
}

protected function getOption($name, $default = null)
Copy link
Owner

Choose a reason for hiding this comment

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

Again, some dockblocks would help keep the class consistent.

{
foreach ($this->argv as $arg) {
list($flag, $val) = explode('=', $arg);

if ($flag == $name) {
Copy link
Owner

Choose a reason for hiding this comment

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

Please use strict-type checking

Suggested change
if ($flag == $name) {
if ($flag === $name) {

return $val;
}
}

return $default;
}

/**
Expand All @@ -75,19 +102,7 @@ public function __construct(array $argv, CLImate $climate)
*/
protected function isFlagSet($flag)
{
$isFlagSet = false;
$argv = $this->argv;

$key = array_search($flag, $argv, true);
if (false !== $key) {
unset($argv[$key]);
$argv = array_values($argv);

$isFlagSet = true;
}

$this->argv = $argv;
return $isFlagSet;
return in_array($flag, array_values($this->argv));
}

/**
Expand All @@ -112,7 +127,6 @@ public function getExitCode()

/**
* @todo Automatically look at server envs for the travis base branch, if not provided?
* @todo Define custom ruleset from command line argv for runPhpcs()
*/
public function run()
{
Expand All @@ -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.'
Copy link
Owner

Choose a reason for hiding this comment

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

This should be able to fit on the same line now.

);
}

$phpcsOutput = $this->runPhpcs($fileDiff);
$phpcsOutput = $this->runPhpcs($fileDiff, $this->ruleset);

if (is_null($phpcsOutput)) {
$this->error('Unable to run phpcs executable.');
Expand Down Expand Up @@ -248,17 +261,19 @@ protected function getChangedFiles()
protected function getChangedLinesPerFile(array $files)
{
$extract = [];
$pattern = '@@ -[0-9]+(?:,[0-9]+)? \+([0-9]+)(?:,([0-9]+))? @@';
$pattern = '/@@ -[0-9]+(?:,[0-9]+)? \+([0-9]+)(?:,([0-9]+))? @@/';

foreach ($files as $file => $data) {
$command = 'git diff -U0 ' . $this->baseBranch . ' ' . $this->currentBranch . ' ' . $file .
' | grep -P ' . escapeshellarg($pattern);
$lineDiff = shell_exec($command);
$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);
Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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)?

Copy link
Owner

@olivertappin olivertappin Aug 7, 2019

Choose a reason for hiding this comment

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

Yes, that sounds like a great idea. That, or we can automatically detect the environment to decide on the method or command to use.

$lines = array_values($lineDiff[0]);

$linesChanged = [];

foreach ($lines as $line) {
preg_match('/' . $pattern . '/', $line, $matches);
preg_match($pattern, $line, $matches);


$start = $end = (int)$matches[1];

Expand Down