src/snakemake/parser.py (1)

111-169: Fix potential infinite loop and improve line collection logic

The parse_fstring method has several issues that need to be addressed:

1. Potential infinite loop: Line 132 increments line unconditionally even when token_.end[0] might be less than token_.start[0] (though unlikely in practice).
2. Incorrect conditional logic: Line 138 uses > when it should be < to keep the longer content.
3. Line parsing complexity: The nested line iteration (lines 130-142) is overly complex for what should be a simpler operation.

Apply this diff to fix the issues:

 def parse_fstring(self, token: tokenize.TokenInfo):
     """
     only for python >= 3.12, since then python changed the
     parsing manner of f-string, see
     [pep-0701](https://peps.python.org/pep-0701)
 
     Here, we just read where the f-string start and end from tokens.
     Luckily, each token records the content of the line,
     and we can just take what we want there.
     """
     lines_contents = dict()
 
     # collect all lines until the end of the f-string
     def collect_lines(token_: tokenize.TokenInfo):
         lines_contents[token_.start[0]] = token_.line
         # if the token spans multiple lines, we need to collect all of them
         if token_.end[0] != token_.start[0]:
 
             # iterate over all lines that the token spans
             token_string = token_.line
             line = token_.start[0]
-            while line <= token_.end[0]:
+            while line <= token_.end[0] and token_string:
                 end_of_line_index = (token_string.index("\n") + 1) if "\n" in token_string \
                     else len(token_string)
                 lines_content = token_string[: end_of_line_index]
 
                 # don't take the new line content if the one already stored is longer
-                if line not in lines_contents or len(lines_contents[line]) > len(lines_content):
+                if line not in lines_contents or len(lines_contents[line]) < len(lines_content):
                     lines_contents[line] = lines_content
 
                 token_string = token_string[end_of_line_index:]
                 line += 1

src/snakemake/parser.py (1)

133-134: Simplify ternary expression using min()

The ternary expression can be simplified for better readability.

-                end_of_line_index = (token_string.index("\n") + 1) if "\n" in token_string \
-                    else len(token_string)
+                end_of_line_index = (token_string.index("\n") + 1) if "\n" in token_string else len(token_string)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/snakemake/parser.py (3)

121-138: Consider adding edge case handling for malformed f-strings.

While the implementation correctly handles well-formed f-strings, consider adding defensive checks for edge cases where tokens might be malformed or incomplete.

For example, you could add validation after line 138:

         # remainder of the line after the newline character
         token_string = token_string[end_of_line_index:]
         i += 1
+
+    # Ensure we have content for the token's end line
+    if token_.end[0] not in lines_contents:
+        lines_contents[token_.end[0]] = ""

---

153-153: Clarify the comment about the t1 variable.

The inline comment could be more explicit about what t1 represents at this point in the code.

-        f_string_end = t1.end # t1 is the FSTRING_END token closing the outermost f-string
+        f_string_end = t1.end  # t1 is the FSTRING_END token closing the outermost f-string

---

111-163: Consider adding a test case for the multiline f-string fix.

The PR checklist indicates that no test case was added. Given that this fixes a specific bug (#3700), it would be valuable to add a regression test to ensure the issue doesn't reoccur.

Would you like me to help generate a test case that verifies multiline f-strings with literal lines are correctly preserved? This would prevent regression of issue #3700.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/snakemake/parser.py (4)

78-91: Helper looks correct; consider minor doc tweak.

split_token_lines() correctly maps physical line numbers to their full line text with newlines preserved. No functional concerns. As a tiny nit, consider avoiding hard-coded token-type integers in the example to keep the doc resilient across Python versions.

---

136-160: Remove dead code: lines_contents and collect_lines() are unused.

This block builds lines_contents but it’s never read when constructing t. It adds complexity without effect and may confuse future maintainers.

Apply this diff to drop the unused logic:

-        lines_contents = dict()
-
-        def collect_lines(token_: tokenize.TokenInfo):
-            # iterate over all lines that the token spans
-            token_string = token_.line
-            i = token_.start[0]
-            while i <= token_.end[0]:
-                end_of_line_index = (
-                    (token_string.index("\n") + 1)
-                    if "\n" in token_string
-                    else len(token_string)
-                )
-                line_content = token_string[:end_of_line_index]
-
-                # don't take the new line content if the one already stored is longer
-                if i not in lines_contents or len(lines_contents[i]) < len(
-                    line_content
-                ):
-                    lines_contents[i] = line_content
-
-                # remainder of the line after the newline character
-                token_string = token_string[end_of_line_index:]
-                i += 1
-
-        # Collect lines for the first and subsequent tokens that are part of the f-string
-        collect_lines(token)

---

162-162: Remove unused variable related_lines.

Static analysis is right: this local is never used.

Apply this diff:

-        related_lines = token.start[0]

---

166-167: Make end-index computation explicit to avoid negative slicing subtlety.

t1.end[1] - len(t1.line) relies on negative indexing semantics against s, which is correct but non-obvious. Computing the absolute end offset improves clarity and reduces off-by-one risks during future edits. While here, you can safely update the lines map on every iteration; values for the same physical line are identical across tokens (TokenInfo.line is the full physical line), so overriding is harmless and simplifies control flow.

Apply this diff:

-            if t1.end[0] not in lines:
-                lines.update(split_token_lines(t1))
+            # Merge the physical lines covered by this token. Overwrites are safe because
+            # TokenInfo.line holds the entire physical line for every token on that line.
+            lines.update(split_token_lines(t1))
@@
-        s = "".join(lines[i] for i in sorted(lines))
-        t = s[token.start[1] : t1.end[1] - len(t1.line)]
+        s = "".join(lines[i] for i in sorted(lines))
+        # Map the tokenizer's end column back into the concatenated buffer 's'.
+        end_in_s = len(s) - len(t1.line) + t1.end[1]
+        t = s[token.start[1] : end_in_s]

Also applies to: 175-179

tests/test_fstring/Snakefile (1)

28-38: Test update captures the regression well; suggest adding the exact minimal reproducer from #3700.

The adjusted expectation verifies that literal lines adjacent to expression braces are preserved. To lock in the specific issue (#3700: dropping a pure-literal middle line), consider adding this assertion as well.

Proposed addition:

@@
 
+# Regression for issue #3700: preserve pure-literal middle line in multi-line f-strings
+assert (
+    f"""abc
+def
+{1+1}"""
+    == """abc
+def
+2"""
+)

I can push this as a follow-up commit if you’d like.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: johanneskoester
PR: snakemake/snakemake#3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in `tests/test_wrapper/Snakefile`, are for testing purposes and do not require updates to the project documentation.

src/snakemake/parser.py (1)

139-140: Consider validating the safety comment.

The commented safety check suggests checking all tokens might be necessary. While the current approach works for the reported issue, edge cases with complex nested f-strings might benefit from this validation.

Consider uncommenting the safety check or documenting why it's not needed:

-            # if t1.end[0] not in lines:  for safety, check all tokens
-            lines.update(split_token_lines(t1))
+            # Update lines dict with all tokens in the f-string to ensure
+            # we capture all line content, even from overlapping tokens
+            lines.update(split_token_lines(t1))

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.