feat: revisit comments, close #348

CHANGELOG.md

@@ -6,6 +6,7 @@ All notable changes to this project will be documented in this file.
 
 ### Formatting Changes + Bug Fixes
 
+-   sqlfmt no longer merges lines that contiain comments, in order to preserve the positional meaning of those comments ([#348](https://github.com/tconbeer/sqlfmt/issues/348) - thank you, [@rileyschack](https://github.com/rileyschack) and [@IanEdington](https://github.com/IanEdington)!).
 -   sqlfmt no longer merges together lines containing multiline jinja blocks unless those lines start with an operator or comma ([#365](https://github.com/tconbeer/sqlfmt/issues/365) - thank you, [@gavlt](https://github.com/gavlt)!).
 -   fixed a bug where adding a jinja end tag (e.g., `{% endif %}`) to a line could cause bad formatting of everything on that line
 

src/sqlfmt/comment.py

@@ -2,7 +2,6 @@
 from dataclasses import dataclass
 from typing import ClassVar, Iterator, Tuple
 
-from sqlfmt.exception import InlineCommentException
 from sqlfmt.token import Token
 
 
@@ -68,20 +67,16 @@ def is_multiline(self) -> bool:
         """
         return "\n" in self.token.token
 
-    def render_inline(self, max_length: int, content_length: int) -> str:
+    @property
+    def was_parsed_inline(self) -> bool:
+        return not self.is_standalone and not self.is_multiline
+
+    def render_inline(self) -> str:
         """
-        For a Comment, returns the string for properly formatting this Comment
-        inline, after content_length characters of non-comment Nodes
+        Renders a comment as an inline comment, assuming it'll fit.
         """
-        if self.is_standalone:
-            raise InlineCommentException("Can't inline standalone comment")
-        else:
-            inline_prefix = " " * 2
-            rendered = inline_prefix + str(self)
-            if content_length + len(rendered) > max_length:
-                raise InlineCommentException("Comment too long to be inlined")
-            else:
-                return inline_prefix + str(self)
+        inline_prefix = " " * 2
+        return f"{inline_prefix}{self}"
 
     def render_standalone(self, max_length: int, prefix: str) -> str:
         """

src/sqlfmt/exception.py

@@ -64,15 +64,6 @@ class SqlfmtControlFlowException(Exception):
     pass
 
 
-class InlineCommentException(SqlfmtControlFlowException):
-    """
-    Raised by the Line class if we try to render a comment
-    inline that is too long
-    """
-
-    pass
-
-
 class StopRulesetLexing(SqlfmtControlFlowException):
     """
     Raised by the Analyzer or one of its actions to indicate

src/sqlfmt/jinjafmt.py

@@ -367,9 +367,11 @@ def format_line(self, line: Line) -> List[Line]:
                         chain(
                             *[
                                 self.format_line(new_line)
-                                for new_line in [
+                                for new_line, _ in [
                                     splitter.split_at_index(line, 0, i, line.comments),
-                                    splitter.split_at_index(line, i, -1, []),
+                                    splitter.split_at_index(
+                                        line, i, -1, [], is_tail=True
+                                    ),
                                 ]
                             ]
                         )

src/sqlfmt/line.py

@@ -2,7 +2,6 @@
 from typing import List, Optional, Tuple
 
 from sqlfmt.comment import Comment
-from sqlfmt.exception import InlineCommentException
 from sqlfmt.node import Node
 from sqlfmt.token import Token
 
@@ -88,39 +87,49 @@ def prefix(self) -> str:
         prefix = INDENT * self.depth[0]
         return prefix
 
+    def has_inline_comment(self, max_length: int) -> bool:
+        """
+        Returns true if the line has an attached comment that was parsed
+        as an inline comment, and if the line and comment are short
+        enough to be rendered inline. Returns false otherwise
+        """
+        content = str(self).rstrip()
+        if self.comments and self.comments[-1].was_parsed_inline:
+            inline_render = self.comments[-1].render_inline()
+            if len(content) + len(inline_render) <= max_length:
+                return True
+        return False
+
     def render_with_comments(self, max_length: int) -> str:
         """
         Returns a string that represents the properly-formatted Line,
         including associated comments
         """
-        content = str(self)
-        rendered = content
-        if len(self.comments) == 1:
-            # standalone or multiline comment
-            if self.nodes[0].is_newline:
-                rendered = f"{self.prefix}{self.comments[0]}"
-            # inline comment
+        content = str(self).rstrip()
+        rendered_lines: List[str] = []
+        for comment in self.comments:
+            if comment.is_multiline or comment.is_standalone:
+                rendered_lines.append(
+                    comment.render_standalone(max_length=max_length, prefix=self.prefix)
+                )
             else:
-                try:
-                    comment = self.comments[0].render_inline(
-                        max_length=max_length, content_length=len(content.rstrip())
-                    )
-                    rendered = f"{content.rstrip()}{comment}"
-                except InlineCommentException:
-                    comment = self.comments[0].render_standalone(
-                        max_length=max_length, prefix=self.prefix
-                    )
-                    rendered = f"{comment}{content}"
-        # wrap comments above; note that str(comment) is newline-terminated
-        elif len(self.comments) > 1:
-            comment = "".join(
-                [
-                    c.render_standalone(max_length=max_length, prefix=self.prefix)
-                    for c in self.comments
-                ]
+                # comment is potentially an inline comment, and we'll handle
+                # that below. Inline comments must be the last comment
+                pass
+
+        if self.has_inline_comment(max_length=max_length):
+            rendered_lines.append(f"{content}{self.comments[-1].render_inline()}")
+        elif self.comments and self.comments[-1].was_parsed_inline:
+            # comment was parsed inline but needs to be written standalone
+            rendered_lines.append(
+                self.comments[-1].render_standalone(
+                    max_length=max_length, prefix=self.prefix
+                )
             )
-            rendered = f"{comment}{content}"
-        return rendered
+            rendered_lines.append(f"{self}")
+        else:
+            rendered_lines.append(f"{self}")
+        return "".join(rendered_lines)
 
     @classmethod
     def from_nodes(

src/sqlfmt/merger.py

@@ -50,9 +50,8 @@ def safe_create_merged_line(self, lines: List[Line]) -> List[Line]:
         except CannotMergeException:
             return lines
 
-    @classmethod
     def _extract_components(
-        cls, lines: Iterable[Line]
+        self, lines: Iterable[Line]
     ) -> Tuple[List[Node], List[Comment]]:
         """
         Given a list of lines, return 2 components:
@@ -67,7 +66,27 @@ def _extract_components(
         final_newline: Optional[Node] = None
         allow_multiline_jinja = True
         has_multiline_jinja = False
+        has_inline_comment_above = False
         for line in lines:
+            # only merge lines with comments if it's a standalone comment
+            # above the first line
+            if line.comments:
+                if nodes or has_inline_comment_above:
+                    raise CannotMergeException(
+                        "Can't merge lines with comments unless the comments "
+                        "are above the first line"
+                    )
+                elif line.has_inline_comment(self.mode.line_length):
+                    has_inline_comment_above = True
+            # make an exception for inline comments followed by
+            # a lonely comma (e.g., leading commas with inline comments)
+            elif has_inline_comment_above:
+                if not line.is_standalone_comma:
+                    raise CannotMergeException(
+                        "Can't merge lines with inline comments unless "
+                        "the following line is a single standalone comma"
+                    )
+
             if has_multiline_jinja and not (
                 line.starts_with_operator or line.starts_with_comma
             ):
@@ -76,7 +95,7 @@ def _extract_components(
                 )
             # skip over newline nodes
             content_nodes = [
-                cls._raise_unmergeable(node, allow_multiline_jinja)
+                self._raise_unmergeable(node, allow_multiline_jinja)
                 for node in line.nodes
                 if not node.is_newline
             ]

src/sqlfmt/splitter.py

@@ -32,25 +32,32 @@ def maybe_split(self, line: Line) -> List[Line]:
                 if head == 0:
                     new_lines.append(line)
                 else:
-                    new_lines.append(self.split_at_index(line, head, i, comments))
+                    new_line, comments = self.split_at_index(
+                        line, head, i, comments, is_tail=True
+                    )
+                    assert (
+                        not comments
+                    ), "Comments must be empty here or we'll drop them"
+                    new_lines.append(new_line)
                 return new_lines
             elif (
                 i > head
                 and not never_split_after
                 and not node.formatting_disabled
                 and (always_split_after or self.maybe_split_before(node))
             ):
-                new_line = self.split_at_index(line, head, i, comments)
+                new_line, comments = self.split_at_index(line, head, i, comments)
                 new_lines.append(new_line)
-                comments = []  # only first split gets original comments
                 head = i
                 # node now follows a new newline node, so we need to update
                 # its previous node (this can impact its depth)
                 node.previous_node = new_line.nodes[-1]
 
             always_split_after, never_split_after = self.maybe_split_after(node)
 
-        new_lines.append(self.split_at_index(line, head, -1, comments))
+        new_line, comments = self.split_at_index(line, head, -1, comments, is_tail=True)
+        assert not comments, "Comments must be empty here or we'll drop them"
+        new_lines.append(new_line)
         return new_lines
 
     def maybe_split_before(self, node: Node) -> bool:
@@ -118,23 +125,46 @@ def maybe_split_after(self, node: Node) -> Tuple[bool, bool]:
             return False, False
 
     def split_at_index(
-        self, line: Line, head: int, index: int, comments: List[Comment]
-    ) -> Line:
+        self,
+        line: Line,
+        head: int,
+        index: int,
+        comments: List[Comment],
+        is_tail: bool = False,
+    ) -> Tuple[Line, List[Comment]]:
         """
-        Return a new line comprised of the nodes line[head:index], plus a newline node
+        Return a new line comprised of the nodes line[head:index], plus a newline node.
+
+        Also return a list of comments that need to be included in the tail
+        of the split line. This includes inline comments and if the head
+        is just a comma, all comments.
         """
         if index == -1:
             new_nodes = line.nodes[head:]
         else:
             assert index > head, "Cannot split at start of line!"
             new_nodes = line.nodes[head:index]
 
+        assert new_nodes, "Cannot split a line without nodes!"
+
+        if is_tail:
+            head_comments, tail_comments = comments, []
+        elif comments:
+            if new_nodes[0].is_comma:
+                head_comments, tail_comments = [], comments
+            elif not comments[-1].is_standalone:
+                head_comments, tail_comments = comments[:-1], [comments[-1]]
+            else:
+                head_comments, tail_comments = comments, []
+        else:
+            head_comments, tail_comments = [], []
+
         new_line = Line.from_nodes(
             previous_node=new_nodes[0].previous_node,
             nodes=new_nodes,
-            comments=comments,
+            comments=head_comments,
         )
         if not new_line.nodes[-1].is_newline:
             self.node_manager.append_newline(new_line)
 
-        return new_line
+        return new_line, tail_comments

tests/data/unformatted/101_multiline.sql

@@ -66,7 +66,7 @@ with
  # And this is a nice multiline jinja comment
  # that we will also handle.
 #}
-/* what!?! */
 select *
 from renamed
-where true
+where
+    true  /* what!?! */

tests/data/unformatted/102_lots_of_comments.sql

@@ -29,7 +29,7 @@ select -- not distinct
     another_thing_entirely,
     yet_another_field
     -- another standalone comment
-from a_really_long_table; -- with a super long comment that won't fit here and needs to move up
+from a_really_long_table; -- with a super long comment that won't fit here and needs to move up above the semicolon
 
 -- sometimes we like really long comments
 -- that wrap to many lines. And may even
@@ -39,10 +39,17 @@ from a_really_long_table; -- with a super long comment that won't fit here and n
 select 1
 )))))__SQLFMT_OUTPUT__(((((
 with
-    one as (select 1),  -- short
-    -- too long to be one-lined with the comment inside so it'll have to go above
-    two as (select a_long_enough_field, another_long_enough_field),
-    three as (select 1),  -- short enough
+    one as (
+        select  -- short
+            1
+    ),
+    two as (
+        -- too long to be one-lined with the comment inside so it'll have to go above
+        select a_long_enough_field, another_long_enough_field
+    ),
+    three as (
+        select 1
+    ),  -- short enough
     four as (
         -- too long to be one-lined with the comment inside so it'll have to go above
         -- and be split and wrapped
@@ -55,14 +62,14 @@ select  -- not distinct
     -- with a long comment that needs to wrap above this line
     one_really_long_field_name,
     -- a standalone comment
-    -- with another comment
-    a_short_field,
+    a_short_field,  -- with another comment
     something_else,
     another_thing_entirely,
     yet_another_field
 -- another standalone comment
--- with a super long comment that won't fit here and needs to move up
 from a_really_long_table
+-- with a super long comment that won't fit here and needs to move up above the
+-- semicolon
 ;
 
 -- sometimes we like really long comments

tests/data/unformatted/113_utils_group_by.sql

@@ -26,4 +26,6 @@ select
     field_a,
     count(*)
 from my_table
-where something_is_true {{ dbt_utils.group_by(10) }}  -- todo: keep line break
+where
+    something_is_true
+    {{ dbt_utils.group_by(10) }}  -- todo: keep line break