Repository: sqlfluff/sqlfluff

Issue:
Return non zero exit code on fix when there are lint violations
We use [lefthook](https://github.com/evilmartians/lefthook) in combination with a pre-commit git hook to fix and commit updated dbt models when committing changes to the repository. This works great with the only downside that `poetry run sqlfluff --fix` will return a 0 exit code when it's done fixing even though there are still unfixable lint violations. 

As an enhancement if would be great if sqlfluff could return a non zero exit code on fix when there are still unfixable linting violations.

Discussion:

Patch:
diff --git a/src/sqlfluff/cli/commands.py b/src/sqlfluff/cli/commands.py
index c9a4d5a2d..e5f2d2a37 100644
--- a/src/sqlfluff/cli/commands.py
+++ b/src/sqlfluff/cli/commands.py
@@ -466,6 +466,7 @@ def fix(force, paths, processes, bench=False, fixed_suffix="", logger=None, **kw
     config = get_config(**kwargs)
     lnt, formatter = get_linter_and_formatter(config, silent=fixing_stdin)
     verbose = config.get("verbose")
+    exit_code = 0
 
     formatter.dispatch_config(lnt)
 
@@ -483,13 +484,14 @@ def fix(force, paths, processes, bench=False, fixed_suffix="", logger=None, **kw
         if result.num_violations(types=SQLLintError, fixable=True) > 0:
             stdout = result.paths[0].files[0].fix_string()[0]
         else:
-            if verbose:
-                if templater_error:
-                    click.echo("Fix aborted due to unparseable template variables.")
-                if unfixable_error:
-                    click.echo("Unfixable violations detected.")
             stdout = stdin
 
+        if verbose:
+            if templater_error:
+                click.echo("Fix aborted due to unparseable template variables.")
+            if unfixable_error:
+                click.echo("Unfixable violations detected.")
+
         click.echo(stdout, nl=False)
         sys.exit(1 if templater_error or unfixable_error else 0)
 
@@ -551,19 +553,31 @@ def fix(force, paths, processes, bench=False, fixed_suffix="", logger=None, **kw
                     _completion_message(config)
             elif c == "n":
                 click.echo("Aborting...")
+                exit_code = 1
             else:  # pragma: no cover
                 click.echo("Invalid input, please enter 'Y' or 'N'")
                 click.echo("Aborting...")
+                exit_code = 1
     else:
         click.echo("==== no fixable linting violations found ====")
-        if result.num_violations(types=SQLLintError, fixable=False) > 0:
-            click.echo(  # pragma: no cover
-                "  [{} unfixable linting violations found]".format(
-                    result.num_violations(types=SQLLintError, fixable=False)
-                )
-            )
         _completion_message(config)
 
+    if result.num_violations(types=SQLLintError, fixable=False) > 0:
+        click.echo(
+            "  [{} unfixable linting violations found]".format(
+                result.num_violations(types=SQLLintError, fixable=False)
+            )
+        )
+        exit_code = 1
+
+    if result.num_violations(types=SQLTemplaterError) > 0:
+        click.echo(
+            "  [{} templating errors found]".format(
+                result.num_violations(types=SQLTemplaterError)
+            )
+        )
+        exit_code = 1
+
     if bench:
         click.echo("==== overall timings ====")
         click.echo(cli_table([("Clock time", result.total_time)]))
@@ -572,7 +586,7 @@ def fix(force, paths, processes, bench=False, fixed_suffix="", logger=None, **kw
             click.echo(f"=== {step} ===")
             click.echo(cli_table(timing_summary[step].items()))
 
-    sys.exit(0)
+    sys.exit(exit_code)
 
 
 def _completion_message(config):
diff --git a/util.py b/util.py
index e697d319a..b88c6f876 100644
--- a/util.py
+++ b/util.py
@@ -64,11 +64,11 @@ def benchmark(cmd, runs, from_file):
     commit_hash = None
     post_results = False
     # Try and detect a CI environment
-    if "CIRCLECI" in os.environ:
-        click.echo("Circle CI detected!")
+    if "CI" in os.environ:
+        click.echo("CI detected!")
         # available_vars = [var for var in os.environ.keys()]  # if var.startswith('CIRCLE')
         # click.echo("Available keys: {0!r}".format(available_vars))
-        commit_hash = os.environ.get("CIRCLE_SHA1", None)
+        commit_hash = os.environ.get("GITHUB_SHA", None)
         post_results = True
         click.echo(f"Commit hash is: {commit_hash!r}")
 
@@ -78,15 +78,21 @@ def benchmark(cmd, runs, from_file):
         results = {}
         for benchmark in benchmarks:
             # Iterate through benchmarks
-            click.echo("Starting bechmark: {!r}".format(benchmark["name"]))
+            click.echo("Starting benchmark: {!r}".format(benchmark["name"]))
             t0 = time.monotonic()
             click.echo("===START PROCESS OUTPUT===")
             process = subprocess.run(benchmark["cmd"])
             click.echo("===END PROCESS OUTPUT===")
             t1 = time.monotonic()
             if process.returncode != 0:
-                click.echo(f"Command failed with return code: {process.returncode}")
-                sys.exit(process.returncode)
+                if benchmark["cmd"][0] == "sqlfluff" and benchmark["cmd"][1] == "fix":
+                    # Allow fix to fail as not all our benchmark errors are fixable
+                    click.echo(
+                        f"Fix command failed with return code: {process.returncode}"
+                    )
+                else:
+                    click.echo(f"Command failed with return code: {process.returncode}")
+                    sys.exit(process.returncode)
             else:
                 duration = t1 - t0
                 click.echo(f"Process completed in {duration:.4f}s")


Test patch:
diff --git a/test/cli/commands_test.py b/test/cli/commands_test.py
index e4f229a42..90a4079fa 100644
--- a/test/cli/commands_test.py
+++ b/test/cli/commands_test.py
@@ -20,7 +20,12 @@ from sqlfluff.cli.commands import lint, version, rules, fix, parse, dialects
 
 
 def invoke_assert_code(
-    ret_code=0, args=None, kwargs=None, cli_input=None, mix_stderr=True
+    ret_code=0,
+    args=None,
+    kwargs=None,
+    cli_input=None,
+    mix_stderr=True,
+    output_contains="",
 ):
     """Invoke a command and check return code."""
     args = args or []
@@ -32,6 +37,8 @@ def invoke_assert_code(
     # Output the CLI code for debugging
     print(result.output)
     # Check return codes
+    if output_contains != "":
+        assert output_contains in result.output
     if ret_code == 0:
         if result.exception:
             raise result.exception
@@ -170,30 +177,6 @@ def test__cli__command_lint_stdin(command):
                 "test/fixtures/linter/operator_errors.sql",
             ],
         ),
-        # Check the script doesn't raise an unexpected exception with badly formed files.
-        (fix, ["--rules", "L001", "test/fixtures/cli/fail_many.sql", "-vvvvvvv"], "y"),
-        # Fix with a suffixs
-        (
-            fix,
-            [
-                "--rules",
-                "L001",
-                "--fixed-suffix",
-                "_fix",
-                "test/fixtures/cli/fail_many.sql",
-            ],
-            "y",
-        ),
-        # Fix without specifying rules
-        (
-            fix,
-            [
-                "--fixed-suffix",
-                "_fix",
-                "test/fixtures/cli/fail_many.sql",
-            ],
-            "y",
-        ),
         # Check that ignoring works (also checks that unicode files parse).
         (
             lint,
@@ -215,6 +198,53 @@ def test__cli__command_lint_parse(command):
     invoke_assert_code(args=command)
 
 
+@pytest.mark.parametrize(
+    "command, ret_code",
+    [
+        # Check the script doesn't raise an unexpected exception with badly formed files.
+        (
+            (
+                fix,
+                ["--rules", "L001", "test/fixtures/cli/fail_many.sql", "-vvvvvvv"],
+                "y",
+            ),
+            1,
+        ),
+        # Fix with a suffixs
+        (
+            (
+                fix,
+                [
+                    "--rules",
+                    "L001",
+                    "--fixed-suffix",
+                    "_fix",
+                    "test/fixtures/cli/fail_many.sql",
+                ],
+                "y",
+            ),
+            1,
+        ),
+        # Fix without specifying rules
+        (
+            (
+                fix,
+                [
+                    "--fixed-suffix",
+                    "_fix",
+                    "test/fixtures/cli/fail_many.sql",
+                ],
+                "y",
+            ),
+            1,
+        ),
+    ],
+)
+def test__cli__command_lint_parse_with_retcode(command, ret_code):
+    """Check commands expecting a non-zero ret code."""
+    invoke_assert_code(ret_code=ret_code, args=command)
+
+
 def test__cli__command_lint_warning_explicit_file_ignored():
     """Check ignoring file works when passed explicitly and ignore file is in the same directory."""
     runner = CliRunner()
@@ -407,37 +437,60 @@ def test__cli__command_fix_stdin_safety():
 
 
 @pytest.mark.parametrize(
-    "sql,exit_code",
+    "sql,exit_code,params,output_contains",
     [
-        ("create TABLE {{ params.dsfsdfds }}.t (a int)", 1),  # template error
-        ("create TABLE a.t (a int)", 0),  # fixable error
-        ("create table a.t (a int)", 0),  # perfection
-        ("select col from a join b using (c)", 1),  # unfixable error (using)
+        (
+            "create TABLE {{ params.dsfsdfds }}.t (a int)",
+            1,
+            "-v",
+            "Fix aborted due to unparseable template variables.",
+        ),  # template error
+        ("create TABLE a.t (a int)", 0, "", ""),  # fixable error
+        ("create table a.t (a int)", 0, "", ""),  # perfection
+        (
+            "select col from a join b using (c)",
+            1,
+            "-v",
+            "Unfixable violations detected.",
+        ),  # unfixable error (using)
     ],
 )
-def test__cli__command_fix_stdin_error_exit_code(sql, exit_code):
+def test__cli__command_fix_stdin_error_exit_code(
+    sql, exit_code, params, output_contains
+):
     """Check that the CLI fails nicely if fixing a templated stdin."""
     if exit_code == 0:
-        invoke_assert_code(args=[fix, ("-",)], cli_input=sql)
+        invoke_assert_code(
+            args=[fix, ("-")],
+            cli_input=sql,
+        )
     else:
         with pytest.raises(SystemExit) as exc_info:
-            invoke_assert_code(args=[fix, ("-",)], cli_input=sql)
-
+            invoke_assert_code(
+                args=[fix, (params, "-")],
+                cli_input=sql,
+                output_contains=output_contains,
+            )
         assert exc_info.value.args[0] == exit_code
 
 
 @pytest.mark.parametrize(
-    "rule,fname,prompt,exit_code",
+    "rule,fname,prompt,exit_code,fix_exit_code",
     [
-        ("L001", "test/fixtures/linter/indentation_errors.sql", "y", 0),
-        ("L001", "test/fixtures/linter/indentation_errors.sql", "n", 65),
+        ("L001", "test/fixtures/linter/indentation_errors.sql", "y", 0, 0),
+        ("L001", "test/fixtures/linter/indentation_errors.sql", "n", 65, 1),
     ],
 )
-def test__cli__command__fix_no_force(rule, fname, prompt, exit_code):
+def test__cli__command__fix_no_force(rule, fname, prompt, exit_code, fix_exit_code):
     """Round trip test, using the prompts."""
     with open(fname) as test_file:
         generic_roundtrip_test(
-            test_file, rule, force=False, final_exit_code=exit_code, fix_input=prompt
+            test_file,
+            rule,
+            force=False,
+            final_exit_code=exit_code,
+            fix_input=prompt,
+            fix_exit_code=fix_exit_code,
         )
 
 
diff --git a/test/fixtures/rules/std_rule_cases/L016.yml b/test/fixtures/rules/std_rule_cases/L016.yml
index f59a7726a..352858ea4 100644
--- a/test/fixtures/rules/std_rule_cases/L016.yml
+++ b/test/fixtures/rules/std_rule_cases/L016.yml
@@ -51,6 +51,19 @@ test_fail_line_too_long_with_comments_4:
     rules:
       max_line_length: 80
 
+test_pass_line_too_long_with_comments_ignore_comment_lines:
+  # Same case as above, but should pass as ignore_comment_lines is set to true
+  pass_str: |
+    SELECT
+    c1
+    ,--  the "y variable" and uses_small_subject_line to be the "x variable" in terms of the regression line.
+    c2
+  configs:
+    rules:
+      max_line_length: 80
+      L016:
+        ignore_comment_lines: True
+
 test_fail_line_too_long_only_comments:
   # Check long lines that are only comments are linted correctly
   fail_str: "-- Some really long comments on their own line\n\nSELECT 1"