Repository: sqlfluff/sqlfluff

Issue:
Make comma placement (L019) a fixable lint
This _seems_ like it should be a pretty straightforward implementation -- place the commas before of after depending on the config?

Let me know if it's trickier than that -- if not, I'd be willing to take a stab at it. I think this would be a nice feature in a 0.4.0 release

Current behavior:

experiment.sql
```sql
select
    field,
    field1,
    field2
    , field3
from my_table
```

Lint ✔️ 
```sh
$ sqlfluff lint experiment.sql
== [experiment.sql] FAIL
L:   5 | P:   5 | L019 | Found leading comma. Expected only trailing.
```

Fix 🙅‍♂️ 
```sh
$ sqlfluff fix experiment.sql --rules L019
==== finding fixable violations ====
==== no fixable linting violations found ====
  [1 unfixable linting violations found]
```


Discussion:
NiallRees: Probably best @alanmcruickshank reviews this one as he knows the logic already.
alanmcruickshank: I'll review this this week. It's meaty enough that it may take me some time to pick through.
pwildenhain: > Took me a while to pick apart what you're doing with the `memory` in the rule, but it's a nice approach.

Thanks, I really sat with this one for a while trying to figure out the best approach. Once I saw how you were utilizing `memory` in other fix routines, this is the solution that came to me 💡 

The bug you mentioned is def a possibility, though I think we can cross that bridge 🌉 if and when that issue comes up 🤞 

@NiallRees Appreciate you jumping in 🤜 🤛 

Patch:
diff --git a/src/sqlfluff/core/rules/std.py b/src/sqlfluff/core/rules/std.py
index 98fbf5786..c1f61b63a 100644
--- a/src/sqlfluff/core/rules/std.py
+++ b/src/sqlfluff/core/rules/std.py
@@ -1,7 +1,7 @@
 """Standard SQL Linting Rules."""
 
 import itertools
-from typing import Tuple, List
+from typing import Tuple, List, Dict, Any
 
 from .base import BaseCrawler, LintFix, LintResult, RuleSet
 from .config_info import STANDARD_CONFIG_INFO_DICT
@@ -2016,6 +2016,7 @@ class Rule_L018(BaseCrawler):
         return LintResult()
 
 
+@std_rule_set.document_fix_compatible
 @std_rule_set.document_configuration
 @std_rule_set.register
 class Rule_L019(BaseCrawler):
@@ -2066,33 +2067,116 @@ class Rule_L019(BaseCrawler):
                 return raw_stack[idx]
             idx -= 1
 
-    def _eval(self, segment, raw_stack, **kwargs):
+    def _eval(self, segment, raw_stack, memory, **kwargs):
         """Enforce comma placement.
 
-        If want leading commas, we're looking for trailing commas, so
-        we look for newline segments. If we want trailing commas then
-        we're looking for leading commas, so we look for the comma itself.
+        For leading commas we're looking for trailing commas, so
+        we look for newline segments. For trailing commas we're
+        looking for leading commas, so we look for the comma itself.
+
+        We also want to handle proper whitespace removal/addition. We remove
+        any trailing whitespace after the leading comma, when converting a
+        leading comma to a trailing comma. We add whitespace after the leading
+        comma when converting a trailing comma to a leading comma.
         """
-        if len(raw_stack) >= 1:
-            if self.comma_style == "leading":
-                if segment.is_type("newline"):
-                    # work back and find the last code segment, was it a comma?
-                    last_seg = self._last_code_seg(raw_stack)
-                    if last_seg.is_type("comma"):
-                        return LintResult(
-                            anchor=last_seg,
-                            description="Found trailing comma. Expected only leading.",
-                        )
-            elif self.comma_style == "trailing":
-                if segment.is_type("comma"):
-                    # work back and find the last interesting thing, is the comma the first element?
-                    last_seg = self._last_code_seg(raw_stack)
-                    if last_seg.is_type("newline"):
-                        return LintResult(
-                            anchor=segment,
-                            description="Found leading comma. Expected only trailing.",
-                        )
-        # Otherwise fine
+        if not memory:
+            memory: Dict[str, Any] = {
+                # Trailing comma keys
+                #
+                # Do we have a fix in place for removing a leading
+                # comma violation, and inserting a new trailing comma?
+                "insert_trailing_comma": False,
+                # A list of whitespace segments that come after a
+                # leading comma violation, to be removed during fixing.
+                "whitespace_deletions": None,
+                # The leading comma violation segment to be removed during fixing
+                "last_leading_comma_seg": None,
+                # The newline segment where we're going to insert our new trailing
+                # comma during fixing
+                "anchor_for_new_trailing_comma_seg": None,
+                #
+                # Leading comma keys
+                #
+                # Do we have a fix in place for removing a trailing
+                # comma violation, and inserting a new leading comma?
+                "insert_leading_comma": False,
+                # The trailing comma violation segment to be removed during fixing
+                "last_trailing_comma_segment": None,
+            }
+
+        if self.comma_style == "trailing":
+            # A comma preceded by a new line == a leading comma
+            if segment.is_type("comma"):
+                last_seg = self._last_code_seg(raw_stack)
+                if last_seg.is_type("newline"):
+                    # Recorded where the fix should be applied
+                    memory["last_leading_comma_seg"] = segment
+                    memory["anchor_for_new_trailing_comma_seg"] = last_seg
+                    # Trigger fix routine
+                    memory["insert_trailing_comma"] = True
+                    memory["whitespace_deletions"] = []
+                    return LintResult(memory=memory)
+            # Have we found a leading comma violation?
+            if memory["insert_trailing_comma"]:
+                # Search for trailing whitespace to delete after the leading
+                # comma violation
+                if segment.is_type("whitespace"):
+                    memory["whitespace_deletions"] += [segment]
+                    return LintResult(memory=memory)
+                else:
+                    # We've run out of whitespace to delete, time to fix
+                    last_leading_comma_seg = memory["last_leading_comma_seg"]
+                    return LintResult(
+                        anchor=last_leading_comma_seg,
+                        description="Found leading comma. Expected only trailing.",
+                        fixes=[
+                            LintFix("delete", last_leading_comma_seg),
+                            *[
+                                LintFix("delete", d)
+                                for d in memory["whitespace_deletions"]
+                            ],
+                            LintFix(
+                                "create",
+                                anchor=memory["anchor_for_new_trailing_comma_seg"],
+                                # Reuse the previous leading comma violation to
+                                # create a new trailing comma
+                                edit=last_leading_comma_seg,
+                            ),
+                        ],
+                    )
+
+        elif self.comma_style == "leading":
+            # A new line preceded by a comma == a trailing comma
+            if segment.is_type("newline"):
+                last_seg = self._last_code_seg(raw_stack)
+                if last_seg.is_type("comma"):
+                    # Trigger fix routine
+                    memory["insert_leading_comma"] = True
+                    # Record where the fix should be applied
+                    memory["last_trailing_comma_segment"] = last_seg
+                    return LintResult(memory=memory)
+            # Have we found a trailing comma violation?
+            if memory["insert_leading_comma"]:
+                # Only insert the comma here if this isn't a comment/whitespace segment
+                if segment.is_code:
+                    last_comma_seg = memory["last_trailing_comma_segment"]
+                    # Create whitespace to insert after the new leading comma
+                    new_whitespace_seg = self.make_whitespace(
+                        raw=" ", pos_marker=segment.pos_marker.advance_by(" ")
+                    )
+                    return LintResult(
+                        anchor=last_comma_seg,
+                        description="Found trailing comma. Expected only leading.",
+                        fixes=[
+                            LintFix("delete", anchor=last_comma_seg),
+                            LintFix(
+                                "edit",
+                                anchor=segment,
+                                edit=[last_comma_seg, new_whitespace_seg, segment],
+                            ),
+                        ],
+                    )
+        # Otherwise, no issue
         return None
 
 


Test patch:
diff --git a/test/core/rules/rule_test_cases_test.py b/test/core/rules/rule_test_cases_test.py
index 994ba351a..852669fa9 100644
--- a/test/core/rules/rule_test_cases_test.py
+++ b/test/core/rules/rule_test_cases_test.py
@@ -20,10 +20,9 @@ for path in sorted(glob.glob(test_cases_path)):
 
     y = yaml.safe_load(raw)
 
-    rule = y["rule"]
-    named_tests = {k: v for k, v in y.items() if k != "rule"}
-    ids.extend([rule + "_" + t for t in named_tests])
-    test_cases.extend([RuleTestCase(rule=rule, **v) for k, v in named_tests.items()])
+    rule = y.pop("rule")
+    ids.extend([rule + "_" + t for t in y])
+    test_cases.extend([RuleTestCase(rule=rule, **v) for k, v in y.items()])
 
 
 @pytest.mark.parametrize("test_case", test_cases, ids=ids)
diff --git a/test/core/rules/test_cases/L019.yml b/test/core/rules/test_cases/L019.yml
index d8fa286e8..43891c842 100644
--- a/test/core/rules/test_cases/L019.yml
+++ b/test/core/rules/test_cases/L019.yml
@@ -1,19 +1,18 @@
 rule: L019
 
-test_1:
-  # Leading commas
+leading_comma_violations:
   fail_str: |
     SELECT
       a
       , b
     FROM c
+  fix_str: |
+    SELECT
+      a,
+      b
+    FROM c
 
-  configs:
-    rules:
-      L019:
-        comma_style: trailing
-
-test_2:
+leading_commas_allowed:
   pass_str: |
     SELECT
       a
@@ -25,8 +24,7 @@ test_2:
       L019:
         comma_style: leading
 
-test_3:
-  # Leading commas in with statement
+leading_comma_violations_in_with_statement:
   fail_str: |
     WITH cte_1 as (
         SELECT *
@@ -40,13 +38,21 @@ test_3:
     )
 
     SELECT * FROM table_3
+  fix_str: |
+    WITH cte_1 as (
+        SELECT *
+        FROM table_1
+    ),
 
-  configs:
-    rules:
-      L019:
-        comma_style: trailing
+    cte_2 as (
+
+        SELECT *
+        FROM table_2
+    )
+
+    SELECT * FROM table_3
 
-test_4:
+leading_commas_allowed_in_with_statement:
   pass_str: |
     WITH cte_1 as (
         SELECT *
@@ -65,27 +71,99 @@ test_4:
       L019:
         comma_style: leading
 
-test_5:
-  # Trailing commas
+trailing_comma_violations:
   fail_str: |
     SELECT
         a,
         b
         FROM c
+  fix_str: |
+    SELECT
+        a
+        , b
+        FROM c
 
   configs:
     rules:
       L019:
         comma_style: leading
 
-test_6:
+trailing_commas_allowed:
   pass_str: |
     SELECT
         a,
         b
     FROM c
 
+trailing_comma_fixing_removes_extra_whitespace:
+  fail_str: |
+    SELECT
+        field_1
+        ,   field_2
+        ,field_3,
+        field_4,
+        field_5
+    FROM a
+  fix_str: |
+    SELECT
+        field_1,
+        field_2,
+        field_3,
+        field_4,
+        field_5
+    FROM a
+
+leading_comma_fixing_flows_around_comments:
+  fail_str: |
+    SELECT
+        a.foo
+        
+        -- another comment
+        , a.baz,
+        -- another comment
+         a.bar,
+    
+        -- This is an important comment with awkward line spacing
+    
+       a.foobar
+    
+        /* Which might also
+        be followed by a multiline one */
+        , a.barfoo
+    FROM a
+    WHERE
+        a.field4 in (
+            '1',
+            '2',
+            '3'
+            ,'4'
+        )
+  fix_str: |
+    SELECT
+        a.foo
+        
+        -- another comment
+        , a.baz
+        -- another comment
+         , a.bar
+    
+        -- This is an important comment with awkward line spacing
+    
+       , a.foobar
+    
+        /* Which might also
+        be followed by a multiline one */
+        , a.barfoo
+    FROM a
+    WHERE
+        a.field4 in (
+            '1'
+            , '2'
+            , '3'
+            ,'4'
+        )
+
   configs:
     rules:
       L019:
-        comma_style: trailing
+        comma_style: leading