Repository: alanmcruickshank/sqlfluff

Issue:
Option to output lint result as JSON
I was toying around with creating a vscode linter extension for sqlfluff. The process entails wrapping a `sqlfluff lint` subprocess and parsing its output into the structure expected by vscode's linting API. 

It wouldn't be _very_ difficult to process the stdout as a string, but this reveals a possible need for a `--json` option so that the linting results can be machine readable in general. I am not sure how easy this would be to accomplish but it possible it might be very easy and a quick win.

Happy to work on this if all agree it is a good idea!



Discussion:
nolanbconaway: I figured out how we can print a full JSON payload at the end of the linter run, by adding these lines at the end of the cli command:

```python
        json_records = []
        for result_path in result.paths:
            for path, violations in result_path.violations().items():
                json_records.append({
                    'filepath': path,
                    'violations': [v.get_info_dict() for v in violations]
                })

        click.echo(json.dumps(json_records))
```

So then this sort of thing would be enabled:

```sh
$ sqlfluff lint test/fixtures/linter/indentation_errors.sql test/fixtures/linter/parse_error.sql --out-format json --rules L010 | jq
[
  {
    "filepath": "test/fixtures/linter/indentation_errors.sql",
    "violations": [
      {
        "code": "L010",
        "line_no": 6,
        "line_pos": 10,
        "description": "Inconsistent capitalisation of keywords."
      }
    ]
  },
  {
    "filepath": "test/fixtures/linter/parse_error.sql",
    "violations": [
      {
        "code": "PRS",
        "line_no": 1,
        "line_pos": 1,
        "description": "Found unparsable segment @L001P001: 'SELECT\\n...'"
      }
    ]
  }
]
```

I think this is a big improvement over what's currently implemented. There is one snag that I'd like advice from @alanmcruickshank about--

With this set up, printing can be done either incrementally in Linter (the human-readable case) or all at once at the end of the run (for JSON). So for JSON we need to disable the incremental printing. How do you see this being handled? Some ideas:

1. A `verbosity = -1`  to cancel all human-readable printing?
2. A `silent=True` option which is checked in Linter.
3. An `out_format='json'` option which is checked in Linter.
4. Move all printing to end of the run.

Happy to push what I have; it uses option 3 for now but we can do whatever @alanmcruickshank  wants!
codecov[bot]: # [Codecov](https://codecov.io/gh/alanmcruickshank/sqlfluff/pull/195?src=pr&el=h1) Report
> Merging [#195](https://codecov.io/gh/alanmcruickshank/sqlfluff/pull/195?src=pr&el=desc) into [master](https://codecov.io/gh/alanmcruickshank/sqlfluff/commit/97099ae22db068faaaf6fb4a68d73f0274551e5b?src=pr&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/alanmcruickshank/sqlfluff/pull/195/graphs/tree.svg?width=650&token=maM8kLgCmn&height=150&src=pr)](https://codecov.io/gh/alanmcruickshank/sqlfluff/pull/195?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #195      +/-   ##
==========================================
+ Coverage   90.87%   90.89%   +0.02%     
==========================================
  Files          24       24              
  Lines        3375     3384       +9     
==========================================
+ Hits         3067     3076       +9     
  Misses        308      308
```

| Flag | Coverage Δ | |
|---|---|---|
| #py35 | `90.74% <100%> (+0.02%)` | :arrow_up: |
| #py36 | `90.74% <100%> (+0.02%)` | :arrow_up: |
| #py37 | `90.74% <100%> (+0.02%)` | :arrow_up: |
| #py38 | `90.65% <100%> (+0.02%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/alanmcruickshank/sqlfluff/pull/195?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/sqlfluff/cli/commands.py](https://codecov.io/gh/alanmcruickshank/sqlfluff/pull/195/diff?src=pr&el=tree#diff-c3JjL3NxbGZsdWZmL2NsaS9jb21tYW5kcy5weQ==) | `81.96% <100%> (+0.09%)` | :arrow_up: |
| [src/sqlfluff/linter.py](https://codecov.io/gh/alanmcruickshank/sqlfluff/pull/195/diff?src=pr&el=tree#diff-c3JjL3NxbGZsdWZmL2xpbnRlci5weQ==) | `86.33% <100%> (+0.22%)` | :arrow_up: |
| [src/sqlfluff/errors.py](https://codecov.io/gh/alanmcruickshank/sqlfluff/pull/195/diff?src=pr&el=tree#diff-c3JjL3NxbGZsdWZmL2Vycm9ycy5weQ==) | `95% <100%> (+0.12%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/alanmcruickshank/sqlfluff/pull/195?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/alanmcruickshank/sqlfluff/pull/195?src=pr&el=footer). Last update [97099ae...b2f8dad](https://codecov.io/gh/alanmcruickshank/sqlfluff/pull/195?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

alanmcruickshank: Hi @nolanbconaway - thanks for putting this together.

In principle, I agree with the approach of capturing the output using `for result_path in result.paths:` piping that into a JSON payload and silencing all logging. That makes perfect sense and is in line with what I'd expect. 

In the `parse` command we have the skeleton for this already (the output there is yaml rather than json, but I think more options is good), where I've used the `silent` element of the config to set the logging to be silent for the rest, I think that's the route I'd prefer.

I think that's option 2 in your tables above. I think the advantage of that method is that the logic as to which config variables silence the linter stays in the `command.py` module, and all the rest of the codebase has to do is check the `silent` flag.

How does that sound?
nolanbconaway: @alanmcruickshank That sounds good to me! I am not super familiar with the architecture at large so definitely better to follow your intuitions on this one! I'll submit my changes and ping you when ready for review

Patch:
diff --git a/.gitignore b/.gitignore
index fed2d343..4899559b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -16,6 +16,7 @@ env
 .tox
 venv
 .venv 
+.python-version
 
 # Ignore coverage reports
 .coverage
diff --git a/src/sqlfluff/cli/commands.py b/src/sqlfluff/cli/commands.py
index a8bb8e90..16672d55 100644
--- a/src/sqlfluff/cli/commands.py
+++ b/src/sqlfluff/cli/commands.py
@@ -1,6 +1,7 @@
 """Contains the CLI."""
 
 import sys
+import json
 
 import click
 # For the profiler
@@ -111,8 +112,11 @@ def rules(**kwargs):
 
 @cli.command()
 @common_options
+@click.option('-f', '--format', 'format', default='human',
+              type=click.Choice(['human', 'json'], case_sensitive=False),
+              help='What format to return the lint result in.')
 @click.argument('paths', nargs=-1)
-def lint(paths, **kwargs):
+def lint(paths, format, **kwargs):
     """Lint SQL files via passing a list of files or using stdin.
 
     PATH is the path to a sql file or directory to lint. This can be either a
@@ -132,12 +136,13 @@ def lint(paths, **kwargs):
 
     """
     c = get_config(**kwargs)
-    lnt = get_linter(c)
+    lnt = get_linter(c, silent=format == 'json')
     verbose = c.get('verbose')
 
     config_string = format_config(lnt, verbose=verbose)
     if len(config_string) > 0:
         lnt.log(config_string)
+
     # add stdin if specified via lone '-'
     if ('-',) == paths:
         result = lnt.lint_string_wrapped(sys.stdin.read(), fname='stdin', verbosity=verbose)
@@ -151,6 +156,10 @@ def lint(paths, **kwargs):
             sys.exit(1)
         # Output the final stats
         lnt.log(format_linting_result_footer(result, verbose=verbose))
+
+    if format == 'json':
+        click.echo(json.dumps(result.as_records()))
+
     sys.exit(result.stats()['exit code'])
 
 
diff --git a/src/sqlfluff/errors.py b/src/sqlfluff/errors.py
index 8768703c..fee9577e 100644
--- a/src/sqlfluff/errors.py
+++ b/src/sqlfluff/errors.py
@@ -96,6 +96,17 @@ class SQLBaseError(ValueError):
         """
         return self.rule_code(), self.line_no(), self.line_pos(), self.desc()
 
+    def get_info_dict(self):
+        """Get a dictionary representation of this violation.
+
+        Returns:
+            A `dictionary` with keys (code, line_no, line_pos, description)
+        """
+        return dict(zip(
+            ('code', 'line_no', 'line_pos', 'description'),
+            self.get_info_tuple()
+        ))
+
     def ignore_if_in(self, ignore_iterable):
         """Ignore this violation if it matches the iterable."""
         # Type conversion
diff --git a/src/sqlfluff/linter.py b/src/sqlfluff/linter.py
index 933c2569..4ce17449 100644
--- a/src/sqlfluff/linter.py
+++ b/src/sqlfluff/linter.py
@@ -386,6 +386,20 @@ class LintingResult:
         all_stats['status'] = 'FAIL' if all_stats['violations'] > 0 else 'PASS'
         return all_stats
 
+    def as_records(self):
+        """Return the result as a list of dictionaries.
+
+        Each record contains a key specifying the filepath, and a list of violations. This
+        method is useful for serialization as all objects will be builtin python types
+        (ints, strs).
+        """
+        return [
+            {'filepath': path, 'violations': [v.get_info_dict() for v in violations]}
+            for lintedpath in self.paths
+            for path, violations in lintedpath.violations().items()
+            if violations
+        ]
+
     def persist_changes(self, verbosity=0, output_func=None, **kwargs):
         """Run all the fixes for all the files and return a dict."""
         return self.combine_dicts(


Test patch:
diff --git a/test/cli_commands_test.py b/test/cli_commands_test.py
index e161e4f4..d2c24ac3 100644
--- a/test/cli_commands_test.py
+++ b/test/cli_commands_test.py
@@ -4,6 +4,7 @@ import configparser
 import tempfile
 import os
 import shutil
+import json
 
 # Testing libraries
 import pytest
@@ -210,3 +211,44 @@ def test__cli__command__fix_no_force(rule, fname, prompt, exit_code):
         generic_roundtrip_test(
             test_file, rule, force=False, final_exit_code=exit_code,
             fix_input=prompt)
+
+
+@pytest.mark.parametrize('sql,expected,exit_code', [
+    ('select * from tbl', [], 0),  # empty list if no violations
+    (
+        'SElect * from tbl',
+        [{
+            "filepath": "stdin",
+            "violations": [
+                {
+                    "code": "L010",
+                    "line_no": 1,
+                    "line_pos": 1,
+                    "description": "Inconsistent capitalisation of keywords."
+                }
+            ]
+        }],
+        65
+    )
+])
+def test__cli__command_lint_json_from_stdin(sql, expected, exit_code):
+    """Check an explicit JSON return value for a single error."""
+    result = invoke_assert_code(
+        args=[lint, ('-', '--rules', 'L010', '--format', 'json')],
+        cli_input=sql,
+        ret_code=exit_code
+    )
+    assert json.loads(result.output) == expected
+
+
+def test__cli__command_lint_json_multiple_files():
+    """Check the general format of JSON output for multiple files."""
+    fpath = 'test/fixtures/linter/indentation_errors.sql'
+
+    # note the file is in here twice. two files = two payloads.
+    result = invoke_assert_code(
+        args=[lint, (fpath, fpath, '--format', 'json')],
+        ret_code=65,
+    )
+    result = json.loads(result.output)
+    assert len(result) == 2