Patch.changed_lines parses diff incorrectly for newly-created files

[joshuacwnewton]

While trying to address #46 (comment), I've been testing Lintly in joshuacwnewton/test-github-actions#20.

In this run for commit joshuacwnewton/test-github-actions@eb30524 Lintly exhibits some odd behavior: flake8 finds violations, yet the violations don't carry over to the diff.

Lintly: 2021-07-18 14:44:38,251 [INFO] lintly.builds: Lintly found violations in 1 files
Lintly: 2021-07-18 14:44:38,251 [DEBUG] lintly.backends.github: Sending a get request to /repos/joshuacwnewton/test-github-actions/pulls/20
Lintly: 2021-07-18 14:44:38,392 [INFO] lintly.builds: Lintly found diff violations in 0 files

The command used is from my lint_code.yml:

flake8 | lintly --use-checks --fail-on new --exit-zero --no-post-status --log
                --commit-sha ${{ github.event.pull_request.head.sha }} 
                --pr ${{ github.event.pull_request.number }} 

Debugging why the Lintly can't find diff violations...

---

To debug, I looked at this part of Lintly's codebase:

Lintly/lintly/builds.py

Lines 76 to 79 in 2c7942a

	diff = self.get_pr_diff()
	patch = self.get_pr_patch(diff)
	self._diff_violations = self.find_diff_violations(patch)
	logger.info('Lintly found diff violations in {} files'.format(len(self._diff_violations)))

I ran Lintly locally, and patch.changed_lines looks like this:

[{'file_name': 'python-file.py', 'content': '+import os # sample linting error', 'line_number': 4, 'position': 5}]

That's odd. 'line_number': 4? This file only has a single line. And indeed, if we compare with self._all_violations, we can see that the violations should have the line number line=1:

defaultdict(<class 'list'>, {'python-file.py': [Violation(line=1, column=1, code="F401", message="'os' imported but unused"), Violation(line=1, column=10, code="E261", message="at least two spaces before inline comment"), Violation(line=1, column=33, code="W292", message="no newline at end of file")]})

So, when find_diff_violations() compares line numbers, it incorrectly compares v.line (1) to line['line_number'] (4).

Lintly/lintly/builds.py

Line 108 in 2c7942a

line_violations = [v for v in file_violations if v.line == line['line_number']]

There's the issue, I think!

---

Debugging why the patch line numbers are incorrect...

---

Tracing patch.changed_lines, line_number is incorrectly incremented 3 extra times specifically due to:

Lintly/lintly/patch.py

Lines 65 to 66 in 2c7942a

	elif NOT_REMOVED_OR_NEWLINE_WARNING.search(content) or content == '':
	line_number += 1

Some print-statement debugging:

print(content): diff --git a/python-file.py b/python-file.py
`line_number` incremented to 1 (not removed or newline)
print(content): new file mode 100644
`line_number` incremented to 2 (not removed or newline)
print(content): index 0000000..939757f
`line_number` incremented to 3 (not removed or newline)
print(content): @@ -0,0 +1 @@
`line_number` incremented to 4 (not removed or newline)

So the offending lines are the following:

# Contents of `diff` variable:

# diff --git a/python-file.py b/python-file.py
+ # new file mode 100644
+ # index 0000000..939757f
# --- /dev/null
# +++ b/python-file.py
+ # @@ -0,0 +1 @@
# +import os # sample linting error
# \ No newline at end of file

I believe these are specific to diffs of newly-created files, but are accidentally caught by the regex pattern:

Lintly/lintly/patch.py

Line 10 in 2c7942a

NOT_REMOVED_OR_NEWLINE_WARNING = re.compile(r'^[^-\\]')

---

So there we have it!

To double-check that this issue is specific to newly-created files, I then went ahead and modified the file again. And indeed, Lintly works like a charm, properly carrying over the violations to the diff.