Repository: alanmcruickshank/sqlfluff

Issue:
Unexpected indentation rules in the FROM
❤️  this project! I've been taking it for a spin (with the hopes of using it in our CI pipelines for our SQL transforms).

One unexpected behavior is that subsequent joins are required to be on the same level of indentation as the keywords, *not* the same level of indentation of the preceding table,

test.sql:
```sql
select *
from
    table_1
    inner join table_2 on table_2.join_key = table_1.join_key

```
throws 
> `indentation not consistent with line 1`.  

The only way to satisfy that rule is to put the `inner join` on the same indentation row as the `from` segment:

test.sql:
```sql
select *
from
    table_1
inner join table_2 on table_2.join_key = table_1.join_key

```

This is not what I would expect -- the `inner join` should belong on the same indentation as `table_1`.

FYI, I tested this on a re-built version from the `master` branch this morning.

Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 48aa482b..54dd8cc7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Logging and benchmarking of parse performance as part of the CI pipeline.
 - Parsing of object references with defaults like `my_db..my_table`.
 - Support for the `INTERVAL '4 days'` style interval expression.
+- Configurable indentation for `JOIN` clauses.
 
 ### Changed
 
diff --git a/docs/source/indentation.rst b/docs/source/indentation.rst
index 31483c49..70cae245 100644
--- a/docs/source/indentation.rst
+++ b/docs/source/indentation.rst
@@ -6,12 +6,13 @@ Let's talk about indentation
 If there is one part of building a linter that is going to be controversial
 it's going to be **indentation** (closely followed by **cApiTaLiSaTiOn** 😁).
 
-*Sqlfluff* aims to be *opinionated* here, but also *configurable*. The tool
-will have a default viewpoint and will aim to have views on all of the
-important aspects of SQL layout, but if you (or your organisation) don't
-like those views then we aim to allow enough configuration that you can lint
-in line with your views, and still use *sqlfluff*. For more information on
-how to configure rules to your own viewpoint see :ref:`config`.
+*Sqlfluff* aims to be *opinionated* here, but also *configurable* (see
+:ref:`indentconfig`). The tool will have a default viewpoint and will aim
+to have views on all of the important aspects of SQL layout, but if you
+(or your organisation) don't like those views then we aim to allow enough
+configuration that you can lint in line with your views, and still use
+*sqlfluff*. For more information on how to configure rules to your own
+viewpoint see :ref:`config`.
 
 So, without further ado, here are the principles we think apply to indentation:
 
@@ -145,4 +146,61 @@ So, without further ado, here are the principles we think apply to indentation:
          to the elements of code which they come *before*, not *after*.
 
 
+.. _indentconfig:
+
+Configuring Indentation
+^^^^^^^^^^^^^^^^^^^^^^^
+
+How indentation is linted is controlled in the rules, but what indentation
+is expected to be present is controlled by the parser, and therefore
+configured seperately. One of the key areas for this is the indentation
+of the :code:`JOIN` expression.
+
+Semantically, a :code:`JOIN` expression is part of the :code:`FROM` expression
+and therefore would be expected to be indented. However according to many
+of the most common SQL style guides (including the `fishtown SQL style guide`_
+and the `Mozilla SQL style guide`_) the :code:`JOIN` keyword is expected to at
+the same indent as the :code:`FROM` keyword. By default, *sqlfluff* sides with
+the current consensus, which is to *not* indent the :code:`JOIN` keyword,
+however this is one element which is configurable.
+
+By setting values in the :code:`sqlfluff:indentation` section of your config
+file you can control how this is parsed, for example you may work with an
+indentation similar to that of `Baron Schwartz`_.
+
+By setting your config file to:
+
+.. code-block:: cfg
+
+   [sqlfluff:indentation]
+   indented_joins = True
+
+Then the expected indentation will be:
+
+.. code-block:: sql
+
+   SELECT
+      a, b, c
+   FROM my_table
+      JOIN another_table
+         USING(a)
+
+However if no value for :code:`indented_joins` is set, or if it is set to
+:code:`false` then then following indentation will be expected:
+
+.. code-block:: sql
+
+   SELECT
+      a, b, c
+   FROM my_table
+   JOIN another_table
+      USING(a)
+
+By default, *sqlfluff* aims to follow the indentation most common approach
+to indentation. However, if you have other versions of indentation which are
+supported by published style guides, then please submit an issue on github
+to have that variation supported by *sqlfluff*.
+
 .. _`fishtown SQL style guide`: https://github.com/fishtown-analytics/corp/blob/master/dbt_coding_conventions.md#sql-style-guide
+.. _`Mozilla SQL style guide`: https://docs.telemetry.mozilla.org/concepts/sql_style.html#joins
+.. _`Baron Schwartz`: https://www.xaprb.com/blog/2006/04/26/sql-coding-standards/
diff --git a/src/sqlfluff/config.py b/src/sqlfluff/config.py
index 3cd903e1..6f4cbae6 100644
--- a/src/sqlfluff/config.py
+++ b/src/sqlfluff/config.py
@@ -134,9 +134,12 @@ class ConfigLoader:
                     try:
                         v = float(val)
                     except ValueError:
-                        if val in ['True', 'False']:
-                            v = bool(val)
-                        elif val in ['None', 'none']:
+                        cleaned_val = val.strip().lower()
+                        if cleaned_val in ['true']:
+                            v = True
+                        elif cleaned_val in ['false']:
+                            v = False
+                        elif cleaned_val in ['none']:
                             v = None
                         else:
                             v = val
diff --git a/src/sqlfluff/default_config.cfg b/src/sqlfluff/default_config.cfg
index 58a9910f..30db7617 100644
--- a/src/sqlfluff/default_config.cfg
+++ b/src/sqlfluff/default_config.cfg
@@ -7,6 +7,9 @@ rules = None
 exclude_rules = None
 recurse = 0
 
+[sqlfluff:indentation]
+indented_joins = False
+
 [sqlfluff:templater:jinja:macros]
 # Macros provided as builtins for dbt projects
 dbt_ref = {% macro ref(model_ref) %}{{model_ref}}{% endmacro %}
diff --git a/src/sqlfluff/dialects/dialect_ansi.py b/src/sqlfluff/dialects/dialect_ansi.py
index 4432b171..18d3c21b 100644
--- a/src/sqlfluff/dialects/dialect_ansi.py
+++ b/src/sqlfluff/dialects/dialect_ansi.py
@@ -651,14 +651,14 @@ class FromClauseSegment(BaseSegment):
         # and so should be on a sub-indent of it. That isn't
         # common practice however, so for now it will be assumed
         # to be on the same level as the FROM clause. To change
-        # this behaviour, the Dedent would come after the AnyNumberOf
-        # rather than before. TODO: In future this might be
-        # configurable.
-        Dedent,
+        # this behaviour, set the `indented_joins` config value
+        # to True.
+        Dedent.when(indented_joins=False),
         AnyNumberOf(
             Ref('JoinClauseSegment'),
             optional=True
         ),
+        Dedent.when(indented_joins=True)
     )
 
 
diff --git a/src/sqlfluff/parser/grammar.py b/src/sqlfluff/parser/grammar.py
index 3bbdfab6..d9a9fdae 100644
--- a/src/sqlfluff/parser/grammar.py
+++ b/src/sqlfluff/parser/grammar.py
@@ -821,6 +821,9 @@ class Sequence(BaseGrammar):
             while True:
                 # Is it an indent or dedent?
                 if elem.is_meta:
+                    # Is it actually enabled?
+                    if not elem.is_enabled(parse_context=parse_context):
+                        break
                     # Work out how to find an appropriate pos_marker for
                     # the meta segment.
                     if matched_segments:
@@ -851,7 +854,7 @@ class Sequence(BaseGrammar):
                         return matched_segments + tuple(
                             e(pos_marker=meta_pos_marker)
                             for e in self._elements[idx:]
-                            if e.is_meta
+                            if e.is_meta and e.is_enabled(parse_context=parse_context)
                         )
                     else:
                         # we've got to the end of the sequence without matching all
@@ -1287,14 +1290,14 @@ class Bracketed(BaseGrammar):
         if content_match.is_complete():
             # We don't want to add metas if they're already there, so check.
             # We also only add indents if there *is* content.
-            if content_match.matched_segments and content_match.matched_segments[0].is_meta:
+            if content_match.matched_segments and content_match.matched_segments[0].is_meta and content_match.matched_segments[0].is_enabled(parse_context=parse_context):
                 pre_meta = ()
             elif not content_match.matched_segments:
                 pre_meta = ()
             else:
                 pre_meta = (Indent(pos_marker=content_match.matched_segments[0].get_start_pos_marker()),)
 
-            if end_match.matched_segments and end_match.matched_segments[0].is_meta:
+            if end_match.matched_segments and end_match.matched_segments[0].is_meta and end_match.matched_segments[0].is_enabled(parse_context=parse_context):
                 post_meta = ()
             elif not content_match.matched_segments:
                 post_meta = ()
diff --git a/src/sqlfluff/parser/segments_base.py b/src/sqlfluff/parser/segments_base.py
index e7b0d1b1..6cbc63ff 100644
--- a/src/sqlfluff/parser/segments_base.py
+++ b/src/sqlfluff/parser/segments_base.py
@@ -114,9 +114,10 @@ class ParseContext:
     as before.
     """
 
-    __slots__ = ['match_depth', 'parse_depth', 'verbosity', 'dialect', 'match_segment', 'recurse', 'blacklist']
+    __slots__ = ['match_depth', 'parse_depth', 'verbosity', 'dialect', 'match_segment', 'recurse', 'blacklist', 'indentation_config']
 
-    def __init__(self, dialect=None, verbosity=0, match_depth=0, parse_depth=0, match_segment=None, recurse=True, blacklist=None):
+    def __init__(self, dialect=None, verbosity=0, match_depth=0, parse_depth=0,
+                 match_segment=None, recurse=True, blacklist=None, indentation_config=None):
         # Write all the variables in a DRY way. Yes it's a bit convoluted. Sorry.
         for k in self.__slots__:
             setattr(self, k, locals()[k])
@@ -146,7 +147,14 @@ class ParseContext:
     @classmethod
     def from_config(cls, config):
         """Construct a `ParseContext` from a `FluffConfig`."""
-        return cls(dialect=config.get('dialect_obj'), recurse=config.get('recurse'))
+        indentation_config = config.get_section('indentation') or {}
+        try:
+            indentation_config = {k: bool(v) for k, v in indentation_config.items()}
+        except TypeError:
+            raise TypeError(
+                "One of the configuration keys in the `indentation` section is not True or False: {0!r}".format(
+                    indentation_config))
+        return cls(dialect=config.get('dialect_obj'), recurse=config.get('recurse'), indentation_config=indentation_config)
 
 
 class BaseSegment:
diff --git a/src/sqlfluff/parser/segments_common.py b/src/sqlfluff/parser/segments_common.py
index 99356507..d8bac464 100644
--- a/src/sqlfluff/parser/segments_common.py
+++ b/src/sqlfluff/parser/segments_common.py
@@ -255,6 +255,47 @@ class Indent(RawSegment):
     _case_sensitive = False
     indent_val = 1
     is_meta = True
+    _config_rules = None
+
+    @classmethod
+    def when(cls, **kwargs):
+        """Configure whether this indent/dedent is available given certain rules.
+
+        All we do is override the _config_rules parameter
+        for the class.
+
+        _config_rules should be an iterable of tuples (config, True|False)
+        which determine whether this class is enabled or not. Later elements
+        override earlier ones.
+        """
+        if len(kwargs) > 1:
+            raise ValueError("More than one condition specified for {0!r}. [{1!r}]".format(
+                cls, kwargs))
+        # Sorcery (but less to than on KeywordSegment)
+        return type(
+            cls.__name__,
+            (cls, ),
+            dict(_config_rules=kwargs)
+        )
+
+    @classmethod
+    def is_enabled(cls, parse_context):
+        """Given a certain parse context, determine if this segment is enabled.
+
+        All rules are assumed to be False if not present in the parse_context,
+        and later rules in the config override previous ones.
+        """
+        # All rules are assumed to be False if not present
+        if cls._config_rules is not None:
+            config = parse_context.indentation_config or {}
+            # This looks like an iteration, but there should only be one.
+            for rule, val in cls._config_rules.items():
+                conf_val = config.get(rule, False)
+                if val == conf_val:
+                    return True
+                else:
+                    return False
+        return True
 
     @staticmethod
     def _suffix():


Test patch:
diff --git a/test/rules_std_test.py b/test/rules_std_test.py
index c5ef40b0..fd412b95 100644
--- a/test/rules_std_test.py
+++ b/test/rules_std_test.py
@@ -120,7 +120,26 @@ def assert_rule_pass_in_sql(code, sql, configs=None):
     ('L003', 'pass',
      ("SELECT\n    user_id\nFROM\n    age_data\nJOIN\n    audience_size\n    USING (user_id, list_id)\n"
       "-- We LEFT JOIN because blah\nLEFT JOIN\n    verts\n    USING\n        (user_id)"),
-     None, None)
+     None, None),
+    # Configurable indents work.
+    # a) default
+    ('L003', 'pass', 'SELECT a, b, c\nFROM my_tbl\nLEFT JOIN another_tbl USING(a)', None, None),
+    # b) specific
+    ('L003', 'pass', 'SELECT a, b, c\nFROM my_tbl\nLEFT JOIN another_tbl USING(a)', None,
+     {'indentation': {'indented_joins': False}}),
+    # c) specific True, but passing
+    ('L003', 'pass', 'SELECT a, b, c\nFROM my_tbl\n    LEFT JOIN another_tbl USING(a)', None,
+     {'indentation': {'indented_joins': True}}),
+    # d) specific True, but failing
+    ('L003', 'fail',
+     'SELECT a, b, c\nFROM my_tbl\nLEFT JOIN another_tbl USING(a)',
+     'SELECT a, b, c\nFROM my_tbl\n    LEFT JOIN another_tbl USING(a)',
+     {'indentation': {'indented_joins': True}}),
+    # e) specific False, and failing
+    ('L003', 'fail',
+     'SELECT a, b, c\nFROM my_tbl\n    LEFT JOIN another_tbl USING(a)',
+     'SELECT a, b, c\nFROM my_tbl\nLEFT JOIN another_tbl USING(a)',
+     {'indentation': {'indented_joins': False}})
 ])
 def test__rules__std_string(rule, pass_fail, qry, fixed, configs):
     """Test that a rule passes/fails on a given string.