-
Notifications
You must be signed in to change notification settings - Fork 581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
StandardBear: Add fixing capabilities #1984
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,17 @@ | ||
from collections import defaultdict | ||
import re | ||
from subprocess import Popen, PIPE | ||
from typing import List, Iterable, Tuple | ||
|
||
from coalib.bearlib.abstractions.Linter import linter | ||
from dependency_management.requirements.NpmRequirement import NpmRequirement | ||
from coalib.results.Result import Result | ||
from coalib.results.Diff import Diff | ||
|
||
|
||
@linter(executable='standard', | ||
output_format='regex', | ||
output_regex=r'\s*[^:]+:(?P<line>\d+):(?P<column>\d+):' | ||
r'\s*(?P<message>.+)') | ||
use_stdin=True, | ||
use_stderr=True) | ||
class JSStandardBear: | ||
""" | ||
One JavaScript Style to Rule Them All. | ||
|
@@ -24,5 +30,65 @@ class JSStandardBear: | |
CAN_FIX = {'Formatting'} | ||
SEE_MORE = 'https://standardjs.com/rules.html' | ||
|
||
issue_regex = re.compile( | ||
r'\s*[^:]+:(?P<line>\d+):(?P<column>\d+):' | ||
r'\s*(?P<message>.+)') | ||
|
||
def create_arguments(self, filename, file, config_file): | ||
return (filename, '--verbose') | ||
|
||
@staticmethod | ||
def _get_corrected_code(old_code: List[str]) -> List[str]: | ||
""" | ||
Pipes the code to JSStandard and returns the corrected code. | ||
""" | ||
p = Popen( | ||
('standard', '--stdin', '--fix'), | ||
stdin=PIPE, stdout=PIPE, stderr=PIPE) | ||
p.stdin.write(bytes(''.join(old_code), 'UTF-8')) | ||
out, err = p.communicate() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we have to be careful, this can deadlock if the buffers get full (and because we might operate with larger files, this can happen). Are there alternatives that take care of this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe temporary files instead? I checked some bigger files, standard itself is failing if ~500kB files are piped. I should also check the return code at this point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There should be special functions somewhere to call a program with stdin and retrieve stdout and stderr without deadlocking.
Oh yeah good point 👍 |
||
return out.decode('UTF-8').splitlines(True) | ||
|
||
def _get_issues(self, stdout: str) -> Iterable[Tuple[int, str]]: | ||
""" | ||
Gets the issues from the output of JSStandard. | ||
|
||
The issues get parsed with `self.issue_regex` and merged if they | ||
concern the same line. | ||
|
||
:param stdout: Output from which the issues get parsed. | ||
:return: List of tuples containing the line number and the message. | ||
""" | ||
match_objects = ( | ||
self.issue_regex.match(line) for line in stdout.splitlines()) | ||
issues = ( | ||
match_object.groupdict() | ||
for match_object in match_objects if match_object is not None) | ||
line_number_to_messages = defaultdict(list) | ||
for issue in issues: | ||
line_number = int(issue['line']) | ||
line_number_to_messages[line_number].append(issue['message']) | ||
return ( | ||
(line_number, '\n'.join(messages)) | ||
for line_number, messages | ||
in sorted(line_number_to_messages.items())) | ||
|
||
def process_output(self, output, filename, file): | ||
stdout, stderr = output | ||
corrected_code = None | ||
if '--fix' in stderr: | ||
corrected_code = self._get_corrected_code(file) | ||
if len(file) != len(corrected_code): | ||
corrected_code = None | ||
|
||
for line_number, message in self._get_issues(stdout): | ||
diff = None | ||
if corrected_code: | ||
diff = Diff(file) | ||
diff.modify_line(line_number, corrected_code[line_number-1]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can a patch span multiple lines? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not that I'm aware of. But looking at this again, it's not robust at all. If standards "--fix" would remove a line and add one at some other place, the patches could be wrong. |
||
yield Result.from_values( | ||
origin=self, | ||
message=message, | ||
file=filename, | ||
line=line_number, | ||
diffs={filename: diff}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,12 @@ | ||
from coalib.testing.LocalBearTestHelper import verify_local_bear | ||
from queue import Queue | ||
|
||
from coalib.results.Diff import Diff | ||
from coalib.settings.Section import Section | ||
from coalib.testing.BearTestHelper import generate_skip_decorator | ||
from coalib.testing.LocalBearTestHelper import verify_local_bear | ||
from coalib.testing.LocalBearTestHelper import LocalBearTestHelper | ||
from bears.js.JSStandardBear import JSStandardBear | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unnecessary change ;) |
||
good_file = """ | ||
var foo = { | ||
bar: 1, | ||
|
@@ -77,3 +81,95 @@ | |
bad_file_undef, | ||
bad_file_ifelse, | ||
bad_file_func_name,)) | ||
|
||
|
||
@generate_skip_decorator(JSStandardBear) | ||
class JSStandardBearTestClass(LocalBearTestHelper): | ||
bad_code = """ | ||
(function () { | ||
console.log('wrong indentation') | ||
console.log('wrong indentation and semicolon'); | ||
}()) | ||
""".splitlines(True) | ||
|
||
def setUp(self): | ||
section = Section('name') | ||
self.js_standard_bear = JSStandardBear(section, Queue()) | ||
|
||
def _check_diff(self, result, line_number, corrected_code): | ||
self.assertEqual(1, len(result.affected_code)) | ||
affected_code = result.affected_code[0] | ||
self.assertEqual(line_number, affected_code.start.line) | ||
self.assertEqual(line_number, affected_code.end.line) | ||
|
||
self.assertEqual(1, len(result.diffs)) | ||
diff = list(result.diffs.values())[0] | ||
expected_diff = Diff(self.bad_code) | ||
expected_diff.modify_line(line_number, corrected_code) | ||
self.assertEqual(expected_diff, diff) | ||
|
||
def test_diffs(self): | ||
results = self.check_invalidity( | ||
self.js_standard_bear, | ||
self.bad_code) | ||
self.assertEqual(2, len(results)) | ||
|
||
corrected_code = " console.log('wrong indentation')" | ||
self._check_diff(results[0], 3, corrected_code) | ||
|
||
corrected_code = " console.log('wrong indentation and semicolon')" | ||
self._check_diff(results[1], 4, corrected_code) | ||
|
||
def test_messages(self): | ||
results = self.check_invalidity( | ||
self.js_standard_bear, | ||
self.bad_code) | ||
self.assertEqual(2, len(results)) | ||
self.assertIn('(indent)', results[0].message) | ||
|
||
# Check that messages for lines with multiple issues get merged. | ||
self.assertIn('(indent)', results[1].message) | ||
self.assertIn('(semi)', results[1].message) | ||
|
||
def test_corrected_code(self): | ||
""" | ||
Check that the corrected code is valid. | ||
""" | ||
corrected_code = self.js_standard_bear._get_corrected_code( | ||
self.bad_code) | ||
self.check_validity( | ||
self.js_standard_bear, | ||
corrected_code) | ||
|
||
def test_get_issues_from_output(self): | ||
output = """ | ||
coala-bears/test.js:2:5: Expected indentation of 2 spaces but found 4. (indent) | ||
coala-bears/test.js:3:5: Expected indentation of 2 spaces but found 4. (indent) | ||
coala-bears/test.js:3:51: Extra semicolon. (semi) | ||
""" | ||
issues = list(self.js_standard_bear._get_issues(output)) | ||
line_numbers = [line_number for line_number, _ in issues] | ||
self.assertEqual(line_numbers, [2, 3]) | ||
messages = [message for _, message in issues] | ||
expected_messages = [ | ||
'Expected indentation of 2 spaces but found 4. (indent)', | ||
('Expected indentation of 2 spaces but found 4. (indent)\n' | ||
'Extra semicolon. (semi)')] | ||
self.assertEqual(messages, expected_messages) | ||
|
||
def test_different_number_of_lines(self): | ||
""" | ||
Check a case where the number of lines of the invalid and the | ||
corrected code differs. | ||
""" | ||
bad_code_with_empty_lines = ['\n', '\n'] + self.bad_code | ||
results = self.check_invalidity( | ||
self.js_standard_bear, | ||
bad_code_with_empty_lines) | ||
self.assertEqual(3, len(results)) | ||
self.assertIn('(no-multiple-empty-lines)', results[0].message) | ||
|
||
# When there the number of lines differ, the diff is `None`. | ||
self.assertEqual(1, len(results[0].diffs)) | ||
diff = list(results[0].diffs.values())[0] | ||
self.assertEqual(None, diff) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have to execute
standard
twice to get the corrected code?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want the messages and the corrected code I see no other way.