diff-quality --options with quotes do not pass to subprocess properly

[PamelaM]

To reproduce, I created a test branch and touched one long line in a "migrations" directory. I then ran the example given in the docs:

(addgene-core)pam@bento-local:/projects/addgene-core$ diff-quality --violations=pep8 --options="--exclude='*/migrations*' --statistics" pep8_report.txt
No handlers could be found for logger "diff_cover.tool"
-------------
Diff Quality
Quality Report: pep8
Diff: origin/master...HEAD, staged, and unstaged changes
-------------
src/django/lims/tasks/migrations/0001_initial.py (0.0%):
    24: E501 line too long (130 > 79 characters)
-------------
Total:   1 line
Violations: 1 line
% Quality: 0%
-------------

Diff Cover Version:

(addgene-core)pam@bento-local:/projects/addgene-core$ pip freeze | grep diff
diff-cover==0.9.5

My analysis:

1. This is a "hard problem" with passing quotes in arguments to Popen (command_runner.py) I think it would work with "shell=True", but that entails a number of security concerns.
2. The tests make sure that the "--options" value is parsed from the diff-quality command line (test_args.py), but there is no integration test of an "--options" with embedded quotes (test_integrations.py only tests --options="--violations_test_file.py"

[Bachmann1234]

Thanks for the report! Ill try and look at it this week and see if I can find a solution. Though I have to admit its a busy time for me right now.

[PamelaM]

One potential option is to add another command line option to the effect of "--use-shell", and let command_runner.py change the args it passes to Popen:

if use_shell:
    cmd = " ".join(cmd)
... = Popen(cmd, ..., shell=use_shell)

[Bachmann1234]

I fear this problem may be more fundamental :-(

I did some experimentation on a dummy project

(venv)  ~/Desktop/issue50   the_test ●  git --no-pager diff
diff --git a/batman/omg.py b/batman/omg.py
index 4035299..347e0cf 100644
--- a/batman/omg.py
+++ b/batman/omg.py
@@ -1 +1,7 @@
+import os
+
 print("dog")
+
+{"dog" :    3}
+
+3+4

(venv)  ~/Desktop/issue50   the_test ●  diff-quality --compare-branch=master  --violations=pep8 --options="--exclude='./batman/' --statistics"
-------------
Diff Quality
Quality Report: pep8
Diff: master...HEAD, staged, and unstaged changes
-------------
batman/omg.py (83.3%):
    5: E203 whitespace before ':'
-------------
Total:   6 lines
Violations: 1 line
% Quality: 83%
-------------

After creating a situation where it seems like diff-quality should be ignoring stuff I played with pep8 by itself

(venv)  ~/Desktop/issue50   the_test ●  pep8 .
./batman/omg.py:5:7: E203 whitespace before ':'

pep8 --exclude='./batman/' .
(nothing)

So far so good right? But here is the thing. Diff cover iterates though files in the diff and runs the command on them

I droped in a debugger here

(venv)  ✘  ~/Desktop/issue50   the_test ●  diff-quality --compare-branch=master  --violations=pep8 --options="--exclude='./batman/' --statistics"
> /Users/bachmann/code/diff-cover/diff_cover/violationsreporters/base.py(158)violations()
-> output, _ = execute(command)
(Pdb) command
['pep8', "--exclude='./batman/' --statistics", b'batman/omg.py

Turns out specifying the file like that causes pep8 to override the exclude (which quite frankly sounds reasonable to me but is causing issues)

(venv)  ✘  ~/Desktop/issue50   the_test ●  pep8 --exclude='./batman/' --statistics batman/omg.py
batman/omg.py:5:7: E203 whitespace before ':'
1       E203 whitespace before ':'

A solution to that problem is not terribly clear to me short of recognizing the exclude command and copying it or restructure diff quality to just run the command once and simply parsing the report (which... it probably should be doing? I don't know. The original implementer may have been trying to help performance?)

But the good news is I have a fairly easy work around for you. If you generate the report first you can pass it into diff quality.

(venv)  ✘  ~/Desktop/issue50   the_test ●  pep8 --exclude='./batman/' . > pep8report.txt
(venv)  ~/Desktop/issue50   the_test ●  diff-quality --compare-branch=master  --violations=pep8 pep8report.txt
-------------
Diff Quality
Quality Report: pep8
Diff: master...HEAD, staged, and unstaged changes
-------------
batman/omg.py (100%)
-------------
Total:   6 lines
Violations: 0 lines
% Quality: 100%
-------------

The excluded file still shows up in the output but the tool passes. Does that help? I don't think I will have a better fix in the short term.

[Bachmann1234]

WOW that got long. Let me TLDR that.

I think the fix would require changes to how diff-quality generates reports that may be good but I am simply not going to get to in the near term.

However, you can work around the issue by generating the report and passing the already completed report to diff-quality. (see example at the end of the giant comment)

[PamelaM]

Sorry, but that doesn't quite work. The diff-quality report doesn't seem to be matching lines properly.

Using your example, nothing matches:

(agc)pam@bento-local:/projects/agc$ pep8 --exclude='*/migrations*,*/vendor*' . > /tmp/pep8-report.txt
(agc)pam@bento-local:/projects/agc$ diff-quality --fail-under=100 --violations=pep8 --compare-branch=master /tmp/pep8-report.txt
-------------
Diff Quality
Quality Report: pep8
Diff: master...HEAD, staged, and unstaged changes
-------------
src/django/addgene/blat/management/commands/giraffe_reanalyze.py (100%)
-------------
Total:   62 lines
Violations: 0 lines
% Quality: 100%
-------------

Using the single command line, it correctly finds the 3 quality errors:

(agc)pam@bento-local:/projects/agc$ diff-quality --fail-under=100 --violations=pep8 --compare-branch=master
-------------
Diff Quality
Quality Report: pep8
Diff: master...HEAD, staged, and unstaged changes
-------------
src/django/addgene/blat/management/commands/giraffe_reanalyze.py (95.2%):
    119: E127 continuation line over-indented for visual indent
    122: E124 closing bracket does not match visual indentation
    136: E222 multiple spaces after operator
-------------
Total:   62 lines
Violations: 3 lines
% Quality: 95%
-------------

Note that those 3 lines are in the pep8 report:

grep giraffe_reanalyze /tmp/pep8-report.txt
./src/django/addgene/blat/management/commands/giraffe_reanalyze.py:119:28: E127 continuation line over-indented for visual indent
./src/django/addgene/blat/management/commands/giraffe_reanalyze.py:122:28: E124 closing bracket does not match visual indentation
./src/django/addgene/blat/management/commands/giraffe_reanalyze.py:136:22: E222 multiple spaces after operator

[Bachmann1234]

::gulp:: ill take a look at this asap

[Bachmann1234]

@PamelaM Can you post the diff with master btw? It could be that these errors already existed in master in which case diff-quality will ignore them.

[Bachmann1234]

heh, just realized you may not be able to do that (no idea if the repo is open source) can you you confirm that these lines are NOT in master?

[PamelaM]

The errors did not exist on master, they were in code that I'd just written (and yes, it's a close-source project: Addgene's django web site)

Also, notice that if I don't pass in a report, diff-quality DOES identify the errors

(agc)pam@bento-local:/projects/agc$ diff-quality --fail-under=100 --violations=pep8 --compare-branch=master
-------------
Diff Quality
Quality Report: pep8
Diff: master...HEAD, staged, and unstaged changes
-------------
src/django/addgene/blat/management/commands/giraffe_reanalyze.py (95.2%):
    119: E127 continuation line over-indented for visual indent
    122: E124 closing bracket does not match visual indentation
    136: E222 multiple spaces after operator
-------------
Total:   62 lines
Violations: 3 lines
% Quality: 95%
-------------

[Bachmann1234]

Got it. Thanks

[Bachmann1234]

Sorry to be dense but I just tried to reproduce this and failed to (Version 0.9.9)

(venv) ~/Desktop/testProject   repor   pep8 --exclude='*/migrations*,*/vendor*' . > /tmp/pep8-report.txt
(venv) ✘  ~/Desktop/testProject   repor  diff-quality --fail-under=100 --violations=pep8 --compare-branch=master /tmp/pep8-report.txt

-------------
Diff Quality
Quality Report: pep8
Diff: master...HEAD, staged, and unstaged changes
-------------
src/django/addgene/management/commands/giraffe_reanalyze.py (66.7%):
    119: E127 continuation line over-indented for visual indent
    122: E124 closing bracket does not match visual indentation
    136: E222 multiple spaces after operator
-------------
Total:   9 lines
Violations: 3 lines
% Quality: 66%
-------------
Failure. Quality is below 100.0%.

I put the reop here https://github.com/Bachmann1234/diff-cover-issue-50/

The branch I was using is repor