Do not fetch file diff if updated file has no messages #54
Conversation
| if ($isNewFile) { | ||
| $debug('Skipping the linting of the orig file version as it is a new file.'); | ||
| } else { | ||
| $debug('Skipping the linting of the orig file version as the new version of the file contains no lint errors.'); |
There was a problem hiding this comment.
This logging is handled by the above throw and the below catch.
| if ($isNewFile) { | ||
| $debug('Skipping the linting of the orig file version as it is a new file.'); | ||
| } else { | ||
| $debug('Skipping the linting of the orig file version as the new version of the file contains no lint errors.'); |
There was a problem hiding this comment.
This logging is handled by the above throw and the below catch.
There was a problem hiding this comment.
Very elegant solution, which allowed a code serving the mere purpose of debugging messaging to be removed. Nice!
| $debug('git status output:', $gitStatusOutput); | ||
| if (! $gitStatusOutput || false === strpos($gitStatusOutput, $gitFile)) { | ||
| throw new ShellException("Cannot get git status for file '{$gitFile}'"); | ||
| return false; |
There was a problem hiding this comment.
I'm not sure how this worked before, since git status file.php will be an empty string for an already existing file.
| $shell->registerCommand("cat 'foobar.php' | phpcs", $this->phpcs->getEmptyResults()->toPhpcsJson()); | ||
|
|
||
| $options = [ | ||
| 'no-cache-git-root' => 1, |
There was a problem hiding this comment.
This allows us to test without the static variable caching operating.
| $hasNewPhpcsMessages = !empty($newFilePhpcsMessages->getMessages()); | ||
|
|
||
| if (! $hasNewPhpcsMessages) { | ||
| throw new NoChangesException("New file '{$svnFile}' has no PHPCS messages; skipping"); |
There was a problem hiding this comment.
It's not an issue of this PR, but we seem to be using "new file" in multiple meanings across the codebase, which might be confusing when reading the output.
Perhaps we should be using "new version of the {$svnFile}" here, similarly to line 272 where "old file version" is used. And limit the usage of "new file" to newly added file? Or perhaps even better, use "newly added file" instead of "new file"?
Edit: The same goes for "old file", where "old version of" might be more appropriate. And while nitpicking about the messaging, perhaps "previous version" might be better. Not sure, tho, if "new" should be replaced by something like "current/working"
There was a problem hiding this comment.
Yeah, I agree! I realized this same issue when working on this PR and I wanted to go through and rename everything, but I decided it would be better for a different PR. I'll get that next.
| $fileName = $shell->getFileNameFromPath($gitFile); | ||
| $newFilePhpcsMessages = PhpcsMessages::fromPhpcsJson($newFilePhpcsOutput, $fileName); | ||
| $hasNewFilePhpcsIssues = !empty($newFilePhpcsMessages->getMessages()); | ||
| $hasNewPhpcsMessages = !empty($newFilePhpcsMessages->getMessages()); |
There was a problem hiding this comment.
I like the new variable name way better than the old one, makes the code more readable.
| if ($isNewFile) { | ||
| $debug('Skipping the linting of the orig file version as it is a new file.'); | ||
| } else { | ||
| $debug('Skipping the linting of the orig file version as the new version of the file contains no lint errors.'); |
There was a problem hiding this comment.
Very elegant solution, which allowed a code serving the mere purpose of debugging messaging to be removed. Nice!
This is a follow-up to #51, which modifies the automatic mode workflows so that they do not run phpcs on the previous version of a file if the current version has no phpcs messages; in this PR we modify them further to also not generate a VCS diff of the file in that case.
Fixes #53
To do: