From 68916468475042bbfca54dab9a9c0663c54e8f2c Mon Sep 17 00:00:00 2001 From: Philip Trauner Date: Thu, 20 Feb 2020 15:27:52 +0100 Subject: [PATCH 1/7] Add 'skip' attribute to 'Config' Structure: {"" : []} Will normally be set through the per-file configuration system, but still has to be part of the global configuration, as said system applies an overlay on top of the global config. --- squabble/config.py | 9 ++++++--- tests/test_config.py | 6 +++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/squabble/config.py b/squabble/config.py index d7dbe3e..4ba88cb 100644 --- a/squabble/config.py +++ b/squabble/config.py @@ -15,7 +15,8 @@ Config = collections.namedtuple('Config', [ 'reporter', 'plugins', - 'rules' + 'rules', + 'skip' ]) @@ -23,7 +24,8 @@ DEFAULT_CONFIG = dict( reporter='plain', plugins=[], - rules={} + rules={}, + skip={} ) PRESETS = { @@ -189,7 +191,8 @@ def load_config(config_file, preset_names=None, reporter_name=None): return Config( reporter=reporter_name or config.get('reporter', base.reporter), plugins=config.get('plugins', base.plugins), - rules=rules + rules=rules, + skip=config.get('skip', {}) ) diff --git a/tests/test_config.py b/tests/test_config.py index eb92e31..2a27645 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -77,7 +77,7 @@ def test_apply_file_config(mock_extract): 'skip_file': False } - orig = config.Config(reporter='', plugins=[], rules={'foo': {}, 'bar': {}}) + orig = config.Config(reporter='', plugins=[], rules={'foo': {}, 'bar': {}}, skip={}) base = copy.deepcopy(orig) modified = config.apply_file_config(base, 'file_name') @@ -96,12 +96,12 @@ def test_apply_file_config_with_skip_file(mock_extract): 'skip_file': True } - orig = config.Config(reporter='', plugins=[], rules={}) + orig = config.Config(reporter='', plugins=[], rules={}, skip={'file_name': []}) base = copy.deepcopy(orig) modified = config.apply_file_config(base, 'file_name') - assert modified is None + assert modified.skip == {'file_name': []} @patch('squabble.config._get_vcs_root') From f603121bb18307e1b254341823286e8d6b334af8 Mon Sep 17 00:00:00 2001 From: Philip Trauner Date: Thu, 20 Feb 2020 15:35:47 +0100 Subject: [PATCH 2/7] Extend '_LintIssue' with attributes pertaining to location shenanigans --- squabble/lint.py | 5 ++++- squabble/reporter.py | 6 +----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/squabble/lint.py b/squabble/lint.py index 11b0b12..ff08383 100644 --- a/squabble/lint.py +++ b/squabble/lint.py @@ -13,7 +13,10 @@ 'node', 'file', 'severity', - 'location' + 'line_text', + 'line', + 'column', + 'abs_location' ]) # Make all the fields nullable diff --git a/squabble/reporter.py b/squabble/reporter.py index df83b10..d26c680 100644 --- a/squabble/reporter.py +++ b/squabble/reporter.py @@ -149,17 +149,13 @@ def _format_message(issue): return issue.message.format() -def _issue_info(issue, file_contents): +def _issue_info(issue): """Return a dictionary of metadata for an issue.""" - line, line_num, column = _issue_to_file_location(issue, file_contents) formatted = _format_message(issue) return { **issue._asdict(), **(issue.message.asdict() if issue.message else {}), - 'line_text': line, - 'line': line_num, - 'column': column, 'message_formatted': formatted, 'severity': issue.severity.name, } From a5142857d532e7de3a0bba7dd6398467c4de4eed Mon Sep 17 00:00:00 2001 From: Philip Trauner Date: Thu, 20 Feb 2020 15:47:04 +0100 Subject: [PATCH 3/7] Extend '_extract_file_rules' to extract 'disable-next-line' instruction --- squabble/config.py | 30 ++++++++++++++++++------------ tests/test_config.py | 12 ++++++++---- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/squabble/config.py b/squabble/config.py index 4ba88cb..36233b5 100644 --- a/squabble/config.py +++ b/squabble/config.py @@ -89,6 +89,11 @@ } } +COMMENT_RE = re.compile( + r'--\s*' + r'(?:squabble-)?(enable|disable)?(-next-line)?' + r'(?::\s*(\w+)(.*?))?' + r'$', re.I) class UnknownPresetException(SquabbleException): """Raised when user tries to apply a preset that isn't defined.""" @@ -241,26 +246,27 @@ def _extract_file_rules(text): rules = { 'enable': {}, 'disable': [], - 'skip_file': False + 'skip_file': False, + 'skip_lines': [] } - comment_re = re.compile( - r'--\s*' - r'(?:squabble-)?(enable|disable)' - r'(?::\s*(\w+)(.*?))?' - r'$', re.I) - - for line in text.splitlines(): + for line_num, line in enumerate(text.splitlines()): line = line.strip() - - m = re.match(comment_re, line) + m = re.match(COMMENT_RE, line) if m is None: continue - action, rule, opts = m.groups() + action, ctx, rule, opts = m.groups() + line_specific = ctx is not None if action == 'disable' and not rule: - rules['skip_file'] = True + if not line_specific: + rules['skip_file'] = True + else: + # Disregard specific rule for now + # Starts with 0, +1 additional line because + # we want to skip the *next* line + rules['skip_lines'].append(line_num + 2) elif action == 'disable': rules['disable'].append(rule) diff --git a/tests/test_config.py b/tests/test_config.py index 2a27645..81e95e7 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -19,7 +19,8 @@ def test_extract_file_rules(): expected = { 'enable': {'foo': {'k1': 'v1', 'k2': 'v2'}}, 'disable': ['abc'], - 'skip_file': False + 'skip_file': False, + 'skip_lines': [] } assert expected == rules @@ -39,7 +40,8 @@ def test_extract_file_rules_with_prefix(): expected = { 'enable': {'foo': {'k1': 'v1', 'k2': 'v2'}}, 'disable': ['abc'], - 'skip_file': True + 'skip_file': True, + 'skip_lines': [] } assert expected == rules @@ -74,7 +76,8 @@ def test_apply_file_config(mock_extract): mock_extract.return_value = { 'enable': {'baz': {'a': 1}}, 'disable': ['bar'], - 'skip_file': False + 'skip_file': False, + 'skip_lines': [] } orig = config.Config(reporter='', plugins=[], rules={'foo': {}, 'bar': {}}, skip={}) @@ -93,7 +96,8 @@ def test_apply_file_config_with_skip_file(mock_extract): mock_extract.return_value = { 'enable': {}, 'disable': [], - 'skip_file': True + 'skip_file': True, + 'skip_lines': [] } orig = config.Config(reporter='', plugins=[], rules={}, skip={'file_name': []}) From 4ca11c2a1287c9b7ddc2bbc15f9e8910e2c8d355 Mon Sep 17 00:00:00 2001 From: Philip Trauner Date: Thu, 20 Feb 2020 15:50:21 +0100 Subject: [PATCH 4/7] Move / adjust '_location_for_issue' + '_issue_to_file_location' '_issue_to_file_location' is now called '_resolve_location', and consumes an absolute position instead of an issue. This is essential when determining the line number for syntax errors. --- squabble/lint.py | 53 +++++++++++++++++++++++++++++++++++++++++ squabble/reporter.py | 56 -------------------------------------------- 2 files changed, 53 insertions(+), 56 deletions(-) diff --git a/squabble/lint.py b/squabble/lint.py index ff08383..2e76c97 100644 --- a/squabble/lint.py +++ b/squabble/lint.py @@ -206,3 +206,56 @@ def report(self, message, node=None, severity=None): # This is filled in later file=None, )) + +def _location_for_issue(issue): + """ + Return the offset into the file for this issue, or None if it + cannot be determined. + """ + if issue.node and issue.node.location != pglast.Missing: + return issue.node.location.value + + if issue.abs_location is not None: + return issue.abs_location + + return None + + +def _resolve_location(location, contents): + """ + Given a location and the contents of the file, + return the ``(line_str, line, column)`` that node is located at, + or ``('', 1, 0)``. + + :param issue: + :type issue: :int: + :param contents: Full contents of the file being linted, as a string. + :type contents: str + + >>> from squabble.lint import LintIssue + + >>> sql = '1234\\n678\\nABCD' + >>> _resolve_location(8, sql) + ('678', 2, 3) + + >>> sql = '1\\r\\n\\r\\n678\\r\\nBCD' + >>> _resolve_location(7, sql) + ('678', 3, 2) + """ + if location is None or location >= len(contents): + return ("", 1, 0) + + # line number is number of newlines in the file before this + # locationation, 1 indexed. + line_num = contents[:location].count("\n") + 1 + + # Search forwards/backwards for the first newline before and first + # newline after this point. + line_start = contents.rfind("\n", 0, location) + 1 + line_end = contents.find("\n", location) + + # Strip out \r so we can treat \r\n and \n the same way + line = contents[line_start:line_end].replace("\r", "") + column = location - line_start + + return (line, line_num, column) \ No newline at end of file diff --git a/squabble/reporter.py b/squabble/reporter.py index d26c680..64b641d 100644 --- a/squabble/reporter.py +++ b/squabble/reporter.py @@ -82,62 +82,6 @@ def report(reporter_name, issues, files): _print_err(line) -def _location_for_issue(issue): - """ - Return the offset into the file for this issue, or None if it - cannot be determined. - """ - if issue.node and issue.node.location != pglast.Missing: - return issue.node.location.value - - return issue.location - - -def _issue_to_file_location(issue, contents): - """ - Given an issue (which may or may not have a :class:`pglast.Node` with a - ``location`` field) and the contents of the file containing that - node, return the ``(line_str, line, column)`` that node is located at, - or ``('', 1, 0)``. - - :param issue: - :type issue: :class:`squabble.lint.LintIssue` - :param contents: Full contents of the file being linted, as a string. - :type contents: str - - >>> from squabble.lint import LintIssue - - >>> issue = LintIssue(location=8, file='foo') - >>> sql = '1234\\n678\\nABCD' - >>> _issue_to_file_location(issue, sql) - ('678', 2, 3) - - >>> issue = LintIssue(location=7, file='foo') - >>> sql = '1\\r\\n\\r\\n678\\r\\nBCD' - >>> _issue_to_file_location(issue, sql) - ('678', 3, 2) - """ - loc = _location_for_issue(issue) - - if loc is None or loc >= len(contents): - return ('', 1, 0) - - # line number is number of newlines in the file before this - # location, 1 indexed. - line_num = contents[:loc].count('\n') + 1 - - # Search forwards/backwards for the first newline before and first - # newline after this point. - line_start = contents.rfind('\n', 0, loc) + 1 - line_end = contents.find('\n', loc) - - # Strip out \r so we can treat \r\n and \n the same way - line = contents[line_start:line_end].replace('\r', '') - column = loc - line_start - - return(line, line_num, column) - - def _print_err(msg): print(msg, file=sys.stderr) From 70448a1de345ffd4ce40396dfd122f1f389de096 Mon Sep 17 00:00:00 2001 From: Philip Trauner Date: Thu, 20 Feb 2020 15:54:11 +0100 Subject: [PATCH 5/7] Add 'file_name' parameter to 'apply_file_config' to facilitate line skips --- squabble/cli.py | 2 +- squabble/config.py | 20 +++++++++++++------- tests/test_config.py | 4 ++-- tests/test_snapshots.py | 4 ++-- 4 files changed, 18 insertions(+), 12 deletions(-) diff --git a/squabble/cli.py b/squabble/cli.py index 87e91cd..877b9c4 100644 --- a/squabble/cli.py +++ b/squabble/cli.py @@ -105,7 +105,7 @@ def run_linter(base_config, paths, expanded): issues = [] for file_name, contents in files: - file_config = config.apply_file_config(base_config, contents) + file_config = config.apply_file_config(base_config, file_name, contents) if file_config is None: continue issues += lint.check_file(file_config, file_name, contents) diff --git a/squabble/config.py b/squabble/config.py index 36233b5..9709135 100644 --- a/squabble/config.py +++ b/squabble/config.py @@ -201,29 +201,35 @@ def load_config(config_file, preset_names=None, reporter_name=None): ) -def apply_file_config(base, contents): +def apply_file_config(base, file_name, contents): """ Given a base configuration object and the contents of a file, return a new config that applies any file-specific rule additions/deletions. - - Returns ``None`` if the file should be skipped. """ # Operate on a copy so we don't mutate the base config file_rules = copy.deepcopy(base.rules) + skips = copy.deepcopy(base.skip) rules = _extract_file_rules(contents) - if rules['skip_file']: - return None - for rule, opts in rules['enable'].items(): file_rules[rule] = opts for rule in rules['disable']: del file_rules[rule] - return base._replace(rules=file_rules) + for line in rules['skip_lines']: + if skips.get(file_name) is None: + skips[file_name] = [] + skips[file_name].append(line) + + # Handle 'skip_file' after 'skip_lines', as it should be seen as + # an override + if rules['skip_file']: + skips = {**skips, file_name: []} + + return base._replace(rules=file_rules, skip=skips) def _extract_file_rules(text): diff --git a/tests/test_config.py b/tests/test_config.py index 81e95e7..48f7e3c 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -83,7 +83,7 @@ def test_apply_file_config(mock_extract): orig = config.Config(reporter='', plugins=[], rules={'foo': {}, 'bar': {}}, skip={}) base = copy.deepcopy(orig) - modified = config.apply_file_config(base, 'file_name') + modified = config.apply_file_config(base, 'file_name', 'file_name') assert modified.rules == {'foo': {}, 'baz': {'a': 1}} @@ -103,7 +103,7 @@ def test_apply_file_config_with_skip_file(mock_extract): orig = config.Config(reporter='', plugins=[], rules={}, skip={'file_name': []}) base = copy.deepcopy(orig) - modified = config.apply_file_config(base, 'file_name') + modified = config.apply_file_config(base, 'file_name', 'file_name') assert modified.skip == {'file_name': []} diff --git a/tests/test_snapshots.py b/tests/test_snapshots.py index 19c1f48..8ae90db 100644 --- a/tests/test_snapshots.py +++ b/tests/test_snapshots.py @@ -42,7 +42,7 @@ def test_snapshot(file_name): pytest.skip('no output configured') base_cfg = config.get_base_config() - cfg = config.apply_file_config(base_cfg, contents) + cfg = config.apply_file_config(base_cfg, file_name, contents) issues = lint.check_file(cfg, file_name, contents) @@ -74,7 +74,7 @@ def test_reporter_sanity(reporter_name): contents = fp.read() files[file_name] = contents - cfg = config.apply_file_config(base_cfg, contents) + cfg = config.apply_file_config(base_cfg, file_name, contents) issues.extend(lint.check_file(cfg, file_name, contents)) reporter.report(reporter_name, issues, files) From 2f9a9d4fa10c27d3aba43f2ce65e7b721da7c43a Mon Sep 17 00:00:00 2001 From: Philip Trauner Date: Thu, 20 Feb 2020 15:55:44 +0100 Subject: [PATCH 6/7] Pass 'skip' attribute of 'Config' to 'Session' Used to determine if an issue should be reported. --- squabble/lint.py | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/squabble/lint.py b/squabble/lint.py index 2e76c97..0c8b695 100644 --- a/squabble/lint.py +++ b/squabble/lint.py @@ -76,7 +76,7 @@ def check_file(config, name, contents): ``name``. """ rules = _configure_rules(config.rules) - s = Session(rules, contents, file_name=name) + s = Session(rules, config.skip, contents, name) return s.lint() @@ -86,15 +86,27 @@ class Session: class exists mainly to hold the list of issues returned by the enabled rules. """ - def __init__(self, rules, sql_text, file_name): + + def __init__(self, rules, skips, sql_text, file_name): self._rules = rules + self._skips = skips self._sql = sql_text self._issues = [] self._file_name = file_name def report_issue(self, issue): - i = issue._replace(file=self._file_name) - self._issues.append(i) + line_text, line, column = _resolve_location( + _location_for_issue(issue), + self._sql + ) + i = issue._replace(file=self._file_name)._replace( + line_text=line_text, line=line, column=column + ) + + should_skip = line in self._skips.get(self._file_name, []) + + if not should_skip: + self._issues.append(i) def lint(self): """ @@ -114,7 +126,7 @@ def lint(self): root_ctx.report_issue(LintIssue( severity=Severity.CRITICAL, message_text=exc.args[0], - location=exc.location + abs_location=exc.location )) return self._issues From f425460ab6da25f93187a314698c2ec7f0580c33 Mon Sep 17 00:00:00 2001 From: Philip Trauner Date: Thu, 20 Feb 2020 15:56:07 +0100 Subject: [PATCH 7/7] Remaining parameter changes --- squabble/cli.py | 3 ++- squabble/reporter.py | 6 +++--- tests/test_snapshots.py | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/squabble/cli.py b/squabble/cli.py index 877b9c4..1b4b37b 100644 --- a/squabble/cli.py +++ b/squabble/cli.py @@ -106,7 +106,8 @@ def run_linter(base_config, paths, expanded): issues = [] for file_name, contents in files: file_config = config.apply_file_config(base_config, file_name, contents) - if file_config is None: + # The file should be skipped if no specific line is ignored + if file_config.skip.get(file_name) == []: continue issues += lint.check_file(file_config, file_name, contents) diff --git a/squabble/reporter.py b/squabble/reporter.py index 64b641d..e7271ba 100644 --- a/squabble/reporter.py +++ b/squabble/reporter.py @@ -116,7 +116,7 @@ def _issue_info(issue): @reporter("plain") def plain_text_reporter(issue, file_contents): """Simple single-line output format that is easily parsed by editors.""" - info = _issue_info(issue, file_contents) + info = _issue_info(issue) return [ _SIMPLE_FORMAT.format(**info) ] @@ -136,7 +136,7 @@ def color_reporter(issue, file_contents): Extension of :func:`squabble.reporter.plain_text_reporter`, uses ANSI color and shows error location. """ - info = _issue_info(issue, file_contents) + info = _issue_info(issue) info['severity'] = '{color}{severity}{reset}'.format( color=_SEVERITY_COLOR[issue.severity], severity=issue.severity.name, @@ -194,7 +194,7 @@ def sqlint_reporter(issue, file_contents): error_level = {Severity.HIGH, Severity.CRITICAL} - info = _issue_info(issue, file_contents) + info = _issue_info(issue) info['severity'] = 'ERROR' if issue.severity in error_level else 'WARNING' return [ diff --git a/tests/test_snapshots.py b/tests/test_snapshots.py index 8ae90db..df0b070 100644 --- a/tests/test_snapshots.py +++ b/tests/test_snapshots.py @@ -49,7 +49,7 @@ def test_snapshot(file_name): assert len(issues) == len(expected) for i, e in zip(issues, expected): - info = reporter._issue_info(i, contents) + info = reporter._issue_info(i) actual = { k: info.get(k) for k in e.keys()