Fixes for fmt: off blocks (#452)

* chore: bump deps * fix #447: early stop all fmt options if fmt is disabled * fix #136: don't format comments in fmt off blocks * fix: clean up newline handling in comments' * fix: restore comment formatting for data tokens
tconbeer · Jul 13, 2023 · 888a1bd · 888a1bd
1 parent fa66ebf
commit 888a1bd
Show file tree

Hide file tree

Showing 13 changed files with 348 additions and 212 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,15 @@ All notable changes to this project will be documented in this file.
 
 ## [Unreleased]
 
+### Bug Fixes
+
+-   Fixes a bug where `--fmt: off` comments could cause an error in formatting a file
+    ([#447](https://github.com/tconbeer/sqlfmt/issues/447) - thank you [@ramonvermeulen](https://github.com/ramonvermeulen)!).
+-   Fixes a bug where some formatting changes were applied to sections of code in
+    `--fmt: off` blocks.
+-   Fixes a bug where comments inside of `--fmt: off` would still be formatted.
+    ([#136](https://github.com/tconbeer/sqlfmt/issues/136)).
+
 ## [0.19.0] - 2023-06-08
 
 ### Bug Fixes

diff --git a/poetry.lock b/poetry.lock
diff --git a/src/sqlfmt/cli.py b/src/sqlfmt/cli.py
@@ -8,7 +8,7 @@
 from sqlfmt.mode import Mode
 
 
-@click.command()
+@click.command()  # type: ignore
 @click.version_option(package_name="shandy-sqlfmt")
 @click.option(
     "--check",

diff --git a/src/sqlfmt/comment.py b/src/sqlfmt/comment.py
@@ -3,7 +3,7 @@
 from typing import ClassVar, Iterator, Optional, Tuple
 
 from sqlfmt.node import Node
-from sqlfmt.token import Token
+from sqlfmt.token import Token, TokenType
 
 
 @dataclass
@@ -24,14 +24,14 @@ def __str__(self) -> str:
         without preceding whitespace, with a single space between the marker
         and the comment text.
         """
-        if self.is_multiline:
-            return f"{self.token.token}\n"
+        if self.is_multiline or self.formatting_disabled:
+            return f"{self.token.token}"
         else:
             marker, comment_text = self._comment_parts()
             if comment_text:
-                return f"{marker} {comment_text}\n"
+                return f"{marker} {comment_text}"
             else:
-                return f"{marker}\n"
+                return f"{marker}"
 
     def __len__(self) -> int:
         return len(str(self))
@@ -77,33 +77,52 @@ def is_c_style(self) -> bool:
     def is_inline(self) -> bool:
         return not self.is_standalone and not self.is_multiline and not self.is_c_style
 
+    @property
+    def formatting_disabled(self) -> bool:
+        if self.previous_node is None:
+            return False
+        else:
+            # comment formatting is only disabled if there is an explicit FMT_OFF token
+            # (i.e., not if node formatting is disabled due to DATA nodes).
+            return any(
+                [
+                    t.type is TokenType.FMT_OFF
+                    for t in self.previous_node.formatting_disabled
+                ]
+            )
+
     def render_inline(self) -> str:
         """
         Renders a comment as an inline comment, assuming it'll fit.
         """
-        return f"{self}"
+        prefix = self.token.prefix if self.formatting_disabled else "  "
+        return f"{prefix}{self}"
 
     def render_standalone(self, max_length: int, prefix: str) -> str:
         """
         For a Comment, returns the string for properly formatting this Comment
         as a standalone comment (on its own line)
         """
-        if self.is_multiline:
+        if self.formatting_disabled:
+            rendered = f"{self.token.prefix}{self}"
+        elif self.is_multiline:
             # todo: split lines, indent each line the same
-            return prefix + str(self)
+            rendered = prefix + str(self)
         else:
             if len(self) + len(prefix) <= max_length:
-                return prefix + str(self)
+                rendered = prefix + str(self)
             else:
                 marker, comment_text = self._comment_parts()
                 if marker in ("--", "#"):
                     available_length = max_length - len(prefix) - len(marker) - 2
                     line_gen = self._split_before(comment_text, available_length)
-                    return "".join(
+                    rendered = "".join(
                         [prefix + marker + " " + txt.strip() + "\n" for txt in line_gen]
                     )
                 else:  # block-style or jinja comment. Don't wrap long lines for now
-                    return prefix + str(self)
+                    rendered = prefix + str(self)
+        nl = "\n"
+        return f"{rendered.rstrip(nl)}\n"
 
     @classmethod
     def _split_before(cls, text: str, max_length: int) -> Iterator[str]:

diff --git a/src/sqlfmt/formatter.py b/src/sqlfmt/formatter.py
@@ -60,13 +60,17 @@ def _dedent_jinja_blocks(self, lines: List[Line]) -> List[Line]:
             if (
                 line.is_standalone_jinja_statement
                 and line.nodes[0].is_closing_jinja_block
+                and not line.formatting_disabled
             ):
                 assert start_node
                 line.nodes[0].open_brackets = start_node.open_brackets
 
             if line.nodes and line.nodes[-1].open_jinja_blocks:
                 start_node = line.nodes[-1].open_jinja_blocks[-1]
-                if len(line.open_brackets) < len(start_node.open_brackets):
+                if (
+                    len(line.open_brackets) < len(start_node.open_brackets)
+                    and not start_node.formatting_disabled
+                ):
                     start_node.open_brackets = line.open_brackets
 
         return lines
@@ -84,7 +88,7 @@ def _remove_extra_blank_lines(self, lines: List[Line]) -> List[Line]:
         for line in lines:
             if line.is_blank_line:
                 max_cnt = 2 if line.depth == (0, 0) else 1
-                if cnt < max_cnt:
+                if cnt < max_cnt or line.formatting_disabled:
                     new_lines.append(line)
                 cnt += 1
             else:
@@ -99,6 +103,7 @@ def format(self, raw_query: Query) -> Query:
         2. Formats jinja tags
         3. Dedents jinja block tags to match their least-indented contents
         4. Merges lines
+        5. Removes extra blank lines
         """
         lines = raw_query.lines
 

diff --git a/src/sqlfmt/jinjafmt.py b/src/sqlfmt/jinjafmt.py
@@ -364,7 +364,7 @@ def format_line(self, line: Line) -> List[Line]:
         split before the node (unless it is already the first node on that line)
         """
         line_length = self.mode.line_length
-        if line.contains_jinja:
+        if not line.formatting_disabled and line.contains_jinja:
             running_length = len(line.prefix)
             for i, node in enumerate(line.nodes):
                 is_blackened = self._format_jinja_node(

diff --git a/src/sqlfmt/line.py b/src/sqlfmt/line.py
@@ -104,7 +104,7 @@ def render_with_comments(self, max_length: int) -> str:
                 inline_comments.append(comment.render_inline())
 
         if inline_comments:
-            rendered_lines.append(f"{content}  {' '.join(inline_comments)}")
+            rendered_lines.append(f"{content}{''.join(inline_comments)}\n")
         else:
             rendered_lines.append(f"{self}")
 

diff --git a/src/sqlfmt/merger.py b/src/sqlfmt/merger.py
@@ -187,6 +187,8 @@ def maybe_merge_lines(self, lines: List[Line]) -> List[Line]:
 
         Returns a new list of Lines
         """
+        if not lines or all([line.formatting_disabled for line in lines]):
+            return lines
 
         try:
             merged_lines = self.create_merged_line(lines)

diff --git a/tests/data/preformatted/006_fmt_off_447.sql b/tests/data/preformatted/006_fmt_off_447.sql
@@ -0,0 +1,24 @@
+-- fmt: off
+with abc as (
+    select
+        *
+		{% if env_var('X') == 'Y' %}
+
+		        ,dense_rank() over(partition by z order by y desc) as foo
+		{% endif %}
+
+    from {{ ref('stg_something')}}
+
+),
+
+abc as (
+    select * from
+    country_trial
+    {% if env_var('X') == 'Y' %}
+
+		where foo = 1
+	{% endif %}
+
+)
+
+select * from dim_something
diff --git a/tests/data/preformatted/007_fmt_off_comments.sql b/tests/data/preformatted/007_fmt_off_comments.sql
@@ -0,0 +1,17 @@
+--fmt: off
+
+--nospace
+--      too many spaces
+--
+--              
+-- very                     very                    long                                        comment
+                --
+                    --  oddly indented!
+            /* multi
+line */
+/* multi
+multi
+                    line */
+select 1--nospace
+union all           --lots  of          space
+select 2 /* c */  /* d */     /*e*/
diff --git a/tests/functional_tests/test_general_formatting.py b/tests/functional_tests/test_general_formatting.py
@@ -13,6 +13,8 @@
         "preformatted/003_literals.sql",
         "preformatted/004_with_select.sql",
         "preformatted/005_fmt_off.sql",
+        "preformatted/006_fmt_off_447.sql",
+        "preformatted/007_fmt_off_comments.sql",
         "preformatted/301_multiline_jinjafmt.sql",
         "preformatted/400_create_table.sql",
         "unformatted/100_select_case.sql",

diff --git a/tests/unit_tests/test_comment.py b/tests/unit_tests/test_comment.py
@@ -3,6 +3,7 @@
 import pytest
 
 from sqlfmt.comment import Comment
+from sqlfmt.node_manager import NodeManager
 from sqlfmt.token import Token, TokenType
 
 
@@ -51,6 +52,22 @@ def multiline_comment() -> Comment:
     return comment
 
 
+@pytest.fixture
+def fmt_disabled_comment() -> Comment:
+    t = Token(type=TokenType.FMT_OFF, prefix="", token="--fmt: off", spos=0, epos=10)
+    mgr = NodeManager(case_sensitive_names=False)
+    node = mgr.create_node(t, None)
+    ct = Token(
+        type=TokenType.COMMENT,
+        prefix="          ",
+        token="--   comment",
+        spos=11,
+        epos=33,
+    )
+    comment = Comment(ct, is_standalone=True, previous_node=node)
+    return comment
+
+
 def test_get_marker(
     short_comment: Comment, short_mysql_comment: Comment, nospace_comment: Comment
 ) -> None:
@@ -70,19 +87,19 @@ def test_comment_parts(
 def test_str_len(
     short_comment: Comment, short_mysql_comment: Comment, nospace_comment: Comment
 ) -> None:
-    assert str(short_comment) == short_comment.token.token + "\n"
-    assert str(short_mysql_comment) == short_mysql_comment.token.token + "\n"
+    assert str(short_comment) == short_comment.token.token
+    assert str(short_mysql_comment) == short_mysql_comment.token.token
     assert str(nospace_comment) == str(short_comment)
 
-    assert len(short_comment) == 17
-    assert len(short_mysql_comment) == 16
-    assert len(nospace_comment) == 17
+    assert len(short_comment) == 16
+    assert len(short_mysql_comment) == 15
+    assert len(nospace_comment) == 16
 
 
 def test_render_inline(
     short_comment: Comment, nospace_comment: Comment, standalone_comment: Comment
 ) -> None:
-    expected = "-- short comment\n"
+    expected = "  -- short comment"
     assert short_comment.render_inline() == expected
     assert nospace_comment.render_inline() == expected
 
@@ -95,15 +112,23 @@ def test_render_inline(
     ],
 )
 def test_render_standalone(short_comment: Comment, prefix: str) -> None:
-    assert short_comment.render_standalone(
-        max_length=88, prefix=prefix
-    ) == prefix + str(short_comment)
+    assert (
+        short_comment.render_standalone(max_length=88, prefix=prefix)
+        == prefix + str(short_comment) + "\n"
+    )
     wrapped_comment = short_comment.render_standalone(max_length=14, prefix=prefix)
     lines = wrapped_comment.splitlines(keepends=True)
     assert lines[0] == prefix + "-- short\n"
     assert lines[1] == prefix + "-- comment\n"
 
 
+def test_render_standalone_formatting_disabled(fmt_disabled_comment: Comment) -> None:
+    assert (
+        fmt_disabled_comment.render_standalone(max_length=88, prefix="")
+        == f"{fmt_disabled_comment.token.prefix}{fmt_disabled_comment.token.token}\n"
+    )
+
+
 def test_render_standalone_wrap_strip_whitespace() -> None:
     txt = "-- foo" + " " * 100 + "bar"
     t = Token(type=TokenType.COMMENT, prefix="", token=txt, spos=0, epos=len(txt))
@@ -112,10 +137,11 @@ def test_render_standalone_wrap_strip_whitespace() -> None:
 
 
 def test_render_multiline(multiline_comment: Comment) -> None:
-    assert multiline_comment.render_standalone(max_length=88, prefix="") == str(
-        multiline_comment
+    assert str(multiline_comment) == "/*\ncomment\n*/"
+    assert (
+        multiline_comment.render_standalone(max_length=88, prefix="")
+        == f"{multiline_comment}\n"
     )
-    assert str(multiline_comment) == "/*\ncomment\n*/\n"
 
 
 @pytest.mark.parametrize(
@@ -136,7 +162,7 @@ def test_split_before(text: str, expected_splits: List[str]) -> None:
 def test_empty_comment() -> None:
     t = Token(type=TokenType.COMMENT, prefix=" ", token="-- ", spos=0, epos=3)
     comment = Comment(t, is_standalone=True, previous_node=None)
-    assert str(comment) == "--\n"
+    assert str(comment) == "--"
 
 
 def test_is_inline(

diff --git a/tests/unit_tests/test_splitter.py b/tests/unit_tests/test_splitter.py
@@ -76,18 +76,18 @@ def test_simple_comment_split(
     expected_comments = [
         (
             "-- not distinct, just an ordinary select here,"
-            " no big deal at all, it's okay really\n"
+            " no big deal at all, it's okay really"
         ),
         "",
-        "-- here is a long comment to be wrapped above this line\n",
-        "-- a short comment\n",
-        "-- standalone for third\n",
-        "-- here is another long comment to be wrapped but not indented\n",
+        "-- here is a long comment to be wrapped above this line",
+        "-- a short comment",
+        "-- standalone for third",
+        "-- here is another long comment to be wrapped but not indented",
         "",
-        "-- another comment that is a little bit too long to stay here\n",
-        "-- this should stay\n",
+        "-- another comment that is a little bit too long to stay here",
+        "-- this should stay",
         "",
-        "-- one last comment\n",
+        "-- one last comment",
     ]
 
     for i, line in enumerate(split_lines):