Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds parser and formatter support for inline formatting directives ( Changes
Sequence Diagram(s)sequenceDiagram
participant Tokenizer as Token Stream
participant Parser
participant Formatter
participant Output
Tokenizer->>Parser: emit tokens & comments
Parser->>Parser: detect `# fmt: off` / modifiers
alt fmt-off encountered
Parser->>Formatter: flush_buffer(exiting_keywords=True)
Parser->>Parser: set fmt_off, sort_off_indent, fmt_off_expected_indent
Parser->>Parser: _consume_fmt_off() collect verbatim region
Parser->>Formatter: flush_fmt_off_region(verbatim)
Formatter->>Output: write buffered + verbatim
end
Tokenizer->>Parser: emit `# fmt: on` or dedent boundary
Parser->>Parser: clear fmt_off state and resume keyword parsing
Parser->>Formatter: resume normal formatting and flushes
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Hocnonsense
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
snakefmt/formatter.py (1)
183-188:⚠️ Potential issue | 🟠 Major
# fmt: off[sort]spans can move to the wrong directive slot.If
self.resultalready holds a preserved off-sort region, Line 185 cachesself.result + param_formattedunder the first sortable directive after# fmt: on.post_process_keyword()then reinserts that whole span at the sorted position of that directive, so a mid-rule disabled region does not stay where it appeared. Please flush/freeze the pending sort cache before entering sort-off, or treat the off-sort span as its own barrier entry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakefmt/formatter.py` around lines 183 - 188, When entering the "sort-off" branch (the condition around fmt_sort_off, in_global_context and keyword_spec in the block that calls format_params), flush any pending sortable-cache held in self.result before combining it with param_formatted and storing into self.keywords; do this by committing the existing self.result as its own barrier (e.g., call the same flush/commit logic that post_process_keyword would use or invoke post_process_keyword() to write out the pending span) or push self.result into self.keywords as a separate entry, then set self.result = "" and only then store the off-sort span (param_formatted) under the new directive slot so the mid-rule disabled region stays in place rather than being moved during post_process_keyword().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 50-77: The TOC's nested numbered list items are not indented
consistently, causing markdownlint MD005; update the README's main numbered list
so each nested level is indented by two spaces (or a consistent number) under
its parent (e.g., indent the subitems under "Install" and the sub-subitems under
"Containers"/"Plug Us") so all nested list levels follow the same indentation
convention and resolve the lint error.
In `@snakefmt/parser/parser.py`:
- Around line 697-706: The Status returned for column-0 formatter directives
currently uses self.cur_indent, which is wrong when the directive appears before
pending DEDENTs; update the Status call in this branch (the one guarded by
fmt_dir and col_nb(token) == 0) to pass 0 for the current indent instead of
self.cur_indent so the directive reports indent 0 (i.e., replace the
self.cur_indent argument in the Status(...) call with 0).
---
Outside diff comments:
In `@snakefmt/formatter.py`:
- Around line 183-188: When entering the "sort-off" branch (the condition around
fmt_sort_off, in_global_context and keyword_spec in the block that calls
format_params), flush any pending sortable-cache held in self.result before
combining it with param_formatted and storing into self.keywords; do this by
committing the existing self.result as its own barrier (e.g., call the same
flush/commit logic that post_process_keyword would use or invoke
post_process_keyword() to write out the pending span) or push self.result into
self.keywords as a separate entry, then set self.result = "" and only then store
the off-sort span (param_formatted) under the new directive slot so the mid-rule
disabled region stays in place rather than being moved during
post_process_keyword().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28559cac-9719-4d89-a1dd-2d42dc380d28
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
README.mdsnakefmt/formatter.pysnakefmt/parser/parser.pysnakefmt/parser/syntax.pytests/test_formatter.py
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
snakefmt/parser/parser.py (1)
695-705:⚠️ Potential issue | 🟠 MajorReturn
cur_indent=0for column-0# fmt:directives.This branch still returns
self.cur_indent, so a top-level directive emitted before pendingDEDENTs keeps the old nested context open. After arun:or other nested Python block, the following top-level section can be parsed as if it were still nested.Minimal fix
return Status( - token, block_indent, self.cur_indent, buffer, False, pythonable + token, block_indent, 0, buffer, False, pythonable )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakefmt/parser/parser.py` around lines 695 - 705, The branch handling column-0 FMT_DIRECTIVE currently returns self.cur_indent which preserves a nested context; change it to return cur_indent=0 for column-0 directives so top-level directives correctly trigger context exit. Specifically, in the block where fmt_dir is set from FMT_DIRECTIVE.from_token(token) and col_nb(token) == 0 (and the disable/next check), construct the Status using 0 instead of self.cur_indent (i.e., Status(token, block_indent, 0, buffer, False, pythonable)), leaving the other arguments and conditional logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@snakefmt/parser/parser.py`:
- Around line 174-190: The handler that clears a "# fmt: on[sort]" currently
sets self.fmt_sort_off = None unconditionally, which wipes the global “sorting
disabled” sentinel (initialized to -1 in Formatter) and can re-enable sorting
when sort_params=False; update the branch in parser.py (the block using
FMT_DIRECTIVE, _check_fmt_on and status.token) so that when
_check_fmt_on(status.token) == "sort" you only clear fmt_sort_off to None if it
is not the global sentinel (i.e., if self.fmt_sort_off != -1), otherwise leave
it as -1 to preserve the global disable state.
- Around line 427-441: When parsing inside _consume_python, don't treat an
off[...] directive with modifiers (e.g., FMT_DIRECTIVE.from_token -> fmt_label
with fmt_label.disable and fmt_label.modifiers) as a global formatter toggle;
instead leave it as a plain comment by not mutating self.fmt_sort_off or
self.fmt_off and by pushing the token back (call self.snakefile.denext(token))
so the main loop can handle it — i.e., change the branch that currently bails on
fmt_label.disable to check for fmt_label.modifiers and, if present, treat the
token as a comment (denext + break) rather than applying/clearing state; keep
existing logic for real fmt: off/on (use _check_fmt_on and split_token_lines)
unchanged.
In `@tests/test_formatter.py`:
- Around line 1690-1696: The test fixture strings (e.g., the snakecode variable)
use unnecessary f-string prefixes though they contain no interpolations; remove
the leading f from those string literals so they become plain strings (e.g.,
change f" run:\n" to " run:\n") and do the same for the other similar fixtures
mentioned (the blocks around the other ranges) to avoid F541/flake8 failures
while keeping the test data identical.
---
Duplicate comments:
In `@snakefmt/parser/parser.py`:
- Around line 695-705: The branch handling column-0 FMT_DIRECTIVE currently
returns self.cur_indent which preserves a nested context; change it to return
cur_indent=0 for column-0 directives so top-level directives correctly trigger
context exit. Specifically, in the block where fmt_dir is set from
FMT_DIRECTIVE.from_token(token) and col_nb(token) == 0 (and the disable/next
check), construct the Status using 0 instead of self.cur_indent (i.e.,
Status(token, block_indent, 0, buffer, False, pythonable)), leaving the other
arguments and conditional logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a591b7e3-0fcf-4e6c-b1e8-d56ecc656a6b
📒 Files selected for processing (2)
snakefmt/parser/parser.pytests/test_formatter.py
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #280 +/- ##
==========================================
- Coverage 97.81% 96.52% -1.29%
==========================================
Files 12 12
Lines 1188 1497 +309
Branches 227 309 +82
==========================================
+ Hits 1162 1445 +283
- Misses 11 24 +13
- Partials 15 28 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (5)
tests/test_formatter.py (2)
2103-2110: Rename unused loop variable to suppress linter warning.The
formattedvariable is not used in the loop body. Rename it to_formattedto indicate it's intentionally unused.Suggested fix
def test_fmt_off_at_start(self): - for code, formatted in ( + for code, _formatted in ( TestSimpleParamFormatting.example_shell_newline, TestSimpleParamFormatting.example_params_newline, TestSimpleParamFormatting.example_input_threads_newline, ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_formatter.py` around lines 2103 - 2110, In test_fmt_off_at_start, the loop unpacks into (code, formatted) but formatted is unused; rename the second loop variable to _formatted to suppress the linter warning (i.e., update the for-loop tuple unpacking in test_fmt_off_at_start that iterates over TestSimpleParamFormatting.example_shell_newline, example_params_newline, and example_input_threads_newline to use (code, _formatted) instead of (code, formatted)).
2144-2150: Same unused variable pattern here.Suggested fix
def test_fmt_off_not_on(self): - for code, formatted in ( + for code, _formatted in ( TestSimpleParamFormatting.example_shell_newline,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_formatter.py` around lines 2144 - 2150, The test test_fmt_off_not_on declares an unused unpacked variable (for code, formatted in ...) causing the "formatted" variable to be unused; update the loop in test_fmt_off_not_on to iterate a single variable (for code in (TestSimpleParamFormatting.example_shell_newline, TestSimpleParamFormatting.example_params_newline, TestSimpleParamFormatting.example_input_threads_newline,): ) and adjust downstream uses to reference only code, or alternatively make each element a (code, formatted) tuple if formatted is actually needed—modify the loop header and elements accordingly in the test function.snakefmt/parser/parser.py (2)
49-64: Consider addingstrict=Trueto zip() for safety.The
split_token_linesfunction useszip()without an explicitstrict=parameter. Since the line range and splitlines should always match in length for valid tokens, addingstrict=Truewould catch any unexpected mismatches during development.Suggested improvement
return zip( - range(token.start[0], token.end[0] + 1), token.line.splitlines(keepends=True) + range(token.start[0], token.end[0] + 1), + token.line.splitlines(keepends=True), + strict=True, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakefmt/parser/parser.py` around lines 49 - 64, The zip call in split_token_lines may silently drop mismatches between the range(token.start[0], token.end[0] + 1) and token.line.splitlines(keepends=True); update the return in split_token_lines to use zip(..., strict=True) so mismatched lengths raise immediately (referencing split_token_lines, token.start, token.end, token.line). Ensure the zip call is the one changed and that the codebase targets a Python version that supports strict=True.
584-626: Typo in method name:_determe_comment_indent.The method name has a typo: "determe" should be "determine".
Suggested fix
- def _determe_comment_indent(self, token: Token) -> int: + def _determine_comment_indent(self, token: Token) -> int:This would also require updating the 6 call sites at lines 425, 549, 606, 636, 643, and 706.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakefmt/parser/parser.py` around lines 584 - 626, Rename the misspelled method _determe_comment_indent to _determine_comment_indent in its definition and update every call site that references self._determe_comment_indent to self._determine_comment_indent; ensure the method signature and docstring remain unchanged and run tests/linters to catch any remaining references (including in parser logic, unit tests, or other modules) and update them accordingly.snakefmt/formatter.py (1)
211-238: Review logic forno_formatting_yetassignment in handle_fmt_off_region.At line 234,
self.no_formatting_yet = bool(self.lagging_comments)will always beFalsebecauseself.lagging_commentswas just cleared to""at line 228. This appears intentional based on the comment at lines 230-232 ("mark that we've emitted content"), but the expression is misleading.Consider simplifying to make the intent explicit:
Suggested clarification
if is_nested_next: - self.no_formatting_yet = bool(self.lagging_comments) + self.no_formatting_yet = False else: self.no_formatting_yet = True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakefmt/formatter.py` around lines 211 - 238, The assignment to self.no_formatting_yet inside handle_fmt_off_region is confusing because bool(self.lagging_comments) will always be False (lagging_comments was cleared earlier); update the logic to make intent explicit by either computing had_lagging = bool(self.lagging_comments) before it is cleared and then using that variable, or simpler: set self.no_formatting_yet = False when is_nested_next (or True/False as the intended explicit value per the surrounding comment), referencing handle_fmt_off_region, self.no_formatting_yet, self.lagging_comments and is_nested_next to locate and change the assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@snakefmt/formatter.py`:
- Around line 211-238: The assignment to self.no_formatting_yet inside
handle_fmt_off_region is confusing because bool(self.lagging_comments) will
always be False (lagging_comments was cleared earlier); update the logic to make
intent explicit by either computing had_lagging = bool(self.lagging_comments)
before it is cleared and then using that variable, or simpler: set
self.no_formatting_yet = False when is_nested_next (or True/False as the
intended explicit value per the surrounding comment), referencing
handle_fmt_off_region, self.no_formatting_yet, self.lagging_comments and
is_nested_next to locate and change the assignment.
In `@snakefmt/parser/parser.py`:
- Around line 49-64: The zip call in split_token_lines may silently drop
mismatches between the range(token.start[0], token.end[0] + 1) and
token.line.splitlines(keepends=True); update the return in split_token_lines to
use zip(..., strict=True) so mismatched lengths raise immediately (referencing
split_token_lines, token.start, token.end, token.line). Ensure the zip call is
the one changed and that the codebase targets a Python version that supports
strict=True.
- Around line 584-626: Rename the misspelled method _determe_comment_indent to
_determine_comment_indent in its definition and update every call site that
references self._determe_comment_indent to self._determine_comment_indent;
ensure the method signature and docstring remain unchanged and run tests/linters
to catch any remaining references (including in parser logic, unit tests, or
other modules) and update them accordingly.
In `@tests/test_formatter.py`:
- Around line 2103-2110: In test_fmt_off_at_start, the loop unpacks into (code,
formatted) but formatted is unused; rename the second loop variable to
_formatted to suppress the linter warning (i.e., update the for-loop tuple
unpacking in test_fmt_off_at_start that iterates over
TestSimpleParamFormatting.example_shell_newline, example_params_newline, and
example_input_threads_newline to use (code, _formatted) instead of (code,
formatted)).
- Around line 2144-2150: The test test_fmt_off_not_on declares an unused
unpacked variable (for code, formatted in ...) causing the "formatted" variable
to be unused; update the loop in test_fmt_off_not_on to iterate a single
variable (for code in (TestSimpleParamFormatting.example_shell_newline,
TestSimpleParamFormatting.example_params_newline,
TestSimpleParamFormatting.example_input_threads_newline,): ) and adjust
downstream uses to reference only code, or alternatively make each element a
(code, formatted) tuple if formatted is actually needed—modify the loop header
and elements accordingly in the test function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c0505cb-8592-4982-a50e-7e623574dc9b
📒 Files selected for processing (4)
README.mdsnakefmt/formatter.pysnakefmt/parser/parser.pytests/test_formatter.py
✅ Files skipped from review due to trivial changes (1)
- README.md
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/test_formatter.py (1)
2103-2110: Unused loop variableformattedcan be renamed to_formatted.The loop unpacks
(code, formatted)but only usescode. Per Python conventions, prefix unused variables with underscore.♻️ Proposed fix
- for code, formatted in ( + for code, _formatted in ( TestSimpleParamFormatting.example_shell_newline, TestSimpleParamFormatting.example_params_newline, TestSimpleParamFormatting.example_input_threads_newline, ):Also applies to line 2145 (
test_fmt_off_not_on).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_formatter.py` around lines 2103 - 2110, In the tests test_fmt_off_at_start and test_fmt_off_not_on, the loop unpacks tuples as (code, formatted) but never uses the formatted variable; rename this unused loop variable to _formatted (i.e., change (code, formatted) to (code, _formatted)) to follow Python conventions and silence linters, locating the changes in the test methods named test_fmt_off_at_start and test_fmt_off_not_on that iterate over TestSimpleParamFormatting.example_* tuples.snakefmt/parser/parser.py (2)
62-64: Consider addingstrict=Truetozip()for defensive programming.The static analysis tool flags that
zip()lacks an explicitstrict=parameter. If the line count from the token's start/end range ever diverges fromsplitlines()output (e.g., due to edge cases in token parsing), a mismatch would silently truncate. Addingstrict=Truewould surface any such inconsistencies immediately.That said, if the current behavior (silently truncating) is intentional for robustness, this can be safely ignored.
♻️ Optional fix
return zip( - range(token.start[0], token.end[0] + 1), token.line.splitlines(keepends=True) + range(token.start[0], token.end[0] + 1), token.line.splitlines(keepends=True), + strict=True, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakefmt/parser/parser.py` around lines 62 - 64, The zip(...) call that returns paired lines for a token should be made strict to catch mismatched lengths: update the return expression that zips range(token.start[0], token.end[0] + 1) with token.line.splitlines(keepends=True) to pass strict=True so any divergence raises immediately (adjust import/compat if targeting older Python versions).
592-592: Minor: method name typo "determe" should be "determine".The method is named
_determe_comment_indentbut should likely be_determine_comment_indent. Since this is used in multiple places, this is a minor naming consistency issue that could be addressed in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakefmt/parser/parser.py` at line 592, Rename the misspelled method `_determe_comment_indent` to `_determine_comment_indent` across the codebase: update the method definition in the class and replace all references/calls to `_determe_comment_indent` with `_determine_comment_indent` (including any unit tests or callers). Ensure any imports, references in docstrings, and usages like in parser logic that expect the old name are updated to the new name to keep behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@snakefmt/parser/parser.py`:
- Line 425: The inline comment contains a typo: replace the word "diabler" with
"disabler" in the comment text that reads "`# fmt: off[` is not actual format
diabler, it affects limited" so it becomes "`# fmt: off[` is not actual format
disabler, it affects limited"; locate that comment in parser.py (the comment
line containing "`# fmt: off[`") and update the spelling only.
In `@tests/test_formatter.py`:
- Line 1828: The fixture name `sort_with_coments` has a typo; rename it to
`sort_with_comments` wherever it's defined and referenced (e.g., in the test
fixture declaration and any usages in tests) to maintain consistency; update the
symbol `sort_with_coments` to `sort_with_comments` throughout the file so
imports/fixtures and assertions continue to resolve.
- Line 2652: Rename the misspelled variable 'fomatter' to 'formatter' where it's
assigned from setup_formatter (search for setup_formatter(...) and the variable
declaration) and update any subsequent usages accordingly; specifically change
the call at the later usage to use formatter.get_formatted() instead of the old
name or method, ensuring all occurrences (assignment and references) match the
corrected 'formatter' identifier and the get_formatted() method call.
---
Nitpick comments:
In `@snakefmt/parser/parser.py`:
- Around line 62-64: The zip(...) call that returns paired lines for a token
should be made strict to catch mismatched lengths: update the return expression
that zips range(token.start[0], token.end[0] + 1) with
token.line.splitlines(keepends=True) to pass strict=True so any divergence
raises immediately (adjust import/compat if targeting older Python versions).
- Line 592: Rename the misspelled method `_determe_comment_indent` to
`_determine_comment_indent` across the codebase: update the method definition in
the class and replace all references/calls to `_determe_comment_indent` with
`_determine_comment_indent` (including any unit tests or callers). Ensure any
imports, references in docstrings, and usages like in parser logic that expect
the old name are updated to the new name to keep behavior unchanged.
In `@tests/test_formatter.py`:
- Around line 2103-2110: In the tests test_fmt_off_at_start and
test_fmt_off_not_on, the loop unpacks tuples as (code, formatted) but never uses
the formatted variable; rename this unused loop variable to _formatted (i.e.,
change (code, formatted) to (code, _formatted)) to follow Python conventions and
silence linters, locating the changes in the test methods named
test_fmt_off_at_start and test_fmt_off_not_on that iterate over
TestSimpleParamFormatting.example_* tuples.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2c4e289-3806-4114-8205-c8c14f2a8459
📒 Files selected for processing (3)
snakefmt/formatter.pysnakefmt/parser/parser.pytests/test_formatter.py
✅ Files skipped from review due to trivial changes (1)
- snakefmt/formatter.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_formatter.py (1)
2103-2110: Rename unused loop variableformattedto_formatted.The loop unpacks
(code, formatted)but only usescode. The same pattern appears at line 2145. Prefix with underscore to indicate intentional discard and silence the Ruff B007 warning.♻️ Proposed fix
def test_fmt_off_at_start(self): - for code, formatted in ( + for code, _formatted in ( TestSimpleParamFormatting.example_shell_newline, TestSimpleParamFormatting.example_params_newline, TestSimpleParamFormatting.example_input_threads_newline, ):Also at line 2145:
def test_fmt_off_not_on(self): - for code, formatted in ( + for code, _formatted in (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_formatter.py` around lines 2103 - 2110, In the test loop inside test_fmt_off_at_start (and the second similar loop later) the unpacked variable named formatted is unused; rename it to _formatted in the tuple unpacking (for example change "for code, formatted in (...)" to "for code, _formatted in (...)" where the tuples use TestSimpleParamFormatting.example_shell_newline, example_params_newline, example_input_threads_newline) to indicate the value is intentionally discarded and silence the Ruff B007 warning.snakefmt/parser/parser.py (1)
49-64: Consider addingstrict=Truetozip()for defensive programming.The
zip()call pairs line numbers with line content. If the ranges ever mismatch (due to edge cases in tokenization),strict=Truewould raise an error immediately rather than silently truncating.♻️ Proposed fix
return zip( - range(token.start[0], token.end[0] + 1), token.line.splitlines(keepends=True) + range(token.start[0], token.end[0] + 1), + token.line.splitlines(keepends=True), + strict=True, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakefmt/parser/parser.py` around lines 49 - 64, The pairing in split_token_lines currently uses zip(range(...), token.line.splitlines(...)) which can silently truncate if the number of line numbers and splitlines differ; update the call in split_token_lines to use zip(..., strict=True) so a mismatch raises immediately, making tokenization edge-case failures visible and easier to debug.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@snakefmt/parser/parser.py`:
- Around line 424-432: The long conditional in the block that checks
fmt_label.modifiers exceeds the 88-char limit; shorten it by extracting
self._determine_comment_indent(token) into a local variable (e.g.,
comment_indent) and/or by splitting the compound condition across lines so the
expression "if not self.fmt_off or ( self.fmt_off[1] != 'region' and
self._determine_comment_indent(token) == self.fmt_off[0] )" is rewritten into
multiple shorter lines while preserving logic; update the condition around
fmt_label.modifiers / self.fmt_off and keep the subsequent
self.snakefile.denext(token) and break behavior unchanged.
---
Nitpick comments:
In `@snakefmt/parser/parser.py`:
- Around line 49-64: The pairing in split_token_lines currently uses
zip(range(...), token.line.splitlines(...)) which can silently truncate if the
number of line numbers and splitlines differ; update the call in
split_token_lines to use zip(..., strict=True) so a mismatch raises immediately,
making tokenization edge-case failures visible and easier to debug.
In `@tests/test_formatter.py`:
- Around line 2103-2110: In the test loop inside test_fmt_off_at_start (and the
second similar loop later) the unpacked variable named formatted is unused;
rename it to _formatted in the tuple unpacking (for example change "for code,
formatted in (...)" to "for code, _formatted in (...)" where the tuples use
TestSimpleParamFormatting.example_shell_newline, example_params_newline,
example_input_threads_newline) to indicate the value is intentionally discarded
and silence the Ruff B007 warning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7bd98b4-3b5f-4471-bdb8-6952c18c9add
📒 Files selected for processing (2)
snakefmt/parser/parser.pytests/test_formatter.py
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
1 similar comment
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Created PR with unit tests: #281 |
|
Request timed out after 900000ms (requestId=b778f470-fd5f-40cf-8eca-6ed6710e5a6b) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
snakefmt/parser/parser.py (1)
62-64: Consider addingstrict=Truetozip()for defensive coding.The
zip()call pairs line numbers with line content. While the lengths should match by token semantics, addingstrict=Truewould catch any unexpected mismatch during development.♻️ Suggested change
return zip( - range(token.start[0], token.end[0] + 1), token.line.splitlines(keepends=True) + range(token.start[0], token.end[0] + 1), token.line.splitlines(keepends=True), + strict=True, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakefmt/parser/parser.py` around lines 62 - 64, Update the zip call that pairs token line numbers with line contents to use strict=True to detect length mismatches at runtime; specifically change the return statement that currently calls zip(range(token.start[0], token.end[0] + 1), token.line.splitlines(keepends=True)) to pass strict=True (i.e., zip(..., ..., strict=True)) so any unexpected mismatch between the range and token.line.splitlines() is raised immediately; ensure the change compiles against the project's minimum Python version.tests/test_formatter.py (1)
2103-2110: Rename unused loop variableformattedto_formatted.The loop iterates over
(code, formatted)tuples but only usescode. Following Python convention, prefix unused variables with underscore.♻️ Suggested change
def test_fmt_off_at_start(self): - for code, formatted in ( + for code, _formatted in ( TestSimpleParamFormatting.example_shell_newline, TestSimpleParamFormatting.example_params_newline, TestSimpleParamFormatting.example_input_threads_newline, ):Also applies to
test_fmt_off_not_onat line 2145.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_formatter.py` around lines 2103 - 2110, In the two tests test_fmt_off_at_start and test_fmt_off_not_on change the unused loop variable name from formatted to _formatted when unpacking the (code, formatted) tuples so the loops read (code, _formatted); update references in those test functions (test_fmt_off_at_start and test_fmt_off_not_on) to use the new _formatted variable name to follow Python convention for unused variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@snakefmt/parser/parser.py`:
- Around line 469-504: Rename the private method _comsume_fmt_off_in_python to
_consume_fmt_off_in_python to fix the typo; update every call site that
references _comsume_fmt_off_in_python (for example the call near where Python
fmt directives are handled) to the new name, and ensure any internal
references/docstrings remain consistent (no behavioral changes).
---
Nitpick comments:
In `@snakefmt/parser/parser.py`:
- Around line 62-64: Update the zip call that pairs token line numbers with line
contents to use strict=True to detect length mismatches at runtime; specifically
change the return statement that currently calls zip(range(token.start[0],
token.end[0] + 1), token.line.splitlines(keepends=True)) to pass strict=True
(i.e., zip(..., ..., strict=True)) so any unexpected mismatch between the range
and token.line.splitlines() is raised immediately; ensure the change compiles
against the project's minimum Python version.
In `@tests/test_formatter.py`:
- Around line 2103-2110: In the two tests test_fmt_off_at_start and
test_fmt_off_not_on change the unused loop variable name from formatted to
_formatted when unpacking the (code, formatted) tuples so the loops read (code,
_formatted); update references in those test functions (test_fmt_off_at_start
and test_fmt_off_not_on) to use the new _formatted variable name to follow
Python convention for unused variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8565f45b-c25e-4065-b0db-738081a008d6
📒 Files selected for processing (3)
snakefmt/formatter.pysnakefmt/parser/parser.pytests/test_formatter.py
🚧 Files skipped from review as they are similar to previous changes (1)
- snakefmt/formatter.py
There was a problem hiding this comment.
🧹 Nitpick comments (3)
snakefmt/parser/parser.py (1)
49-64: Consider addingstrict=Truetozip()for safety.The static analysis flags that
zip()lacks an explicitstrict=parameter. While the line count should theoretically match, addingstrict=Truewould catch any unexpected mismatches during development.💡 Suggested fix
return zip( - range(token.start[0], token.end[0] + 1), token.line.splitlines(keepends=True) + range(token.start[0], token.end[0] + 1), + token.line.splitlines(keepends=True), + strict=True, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakefmt/parser/parser.py` around lines 49 - 64, The zip in split_token_lines can silently drop mismatched sequences; update the return to use zip(..., strict=True) so split_token_lines(token: tokenize.TokenInfo) raises if the range of line numbers (range(token.start[0], token.end[0] + 1)) and token.line.splitlines(keepends=True) lengths differ, preventing silent mismatches during development.snakefmt/formatter.py (2)
127-133: Consider using an explicit exception instead ofassertfor production safety.The
assertstatement can be disabled when Python runs with optimization flags (-Oor-OO). If this invariant fails in production, the code would continue with undefined behavior instead of raising an error.💡 Suggested alternative
- assert re_rematch, ( - "This should always match as we just formatted it with the same " - "regex. If this error is raised, it's a bug in snakefmt's " - "handling of snakemake syntax. Please report this to the " - "developers with the code so we can fix it: " - "https://github.com/snakemake/snakefmt/issues" - ) + if not re_rematch: + raise RuntimeError( + "This should always match as we just formatted it with the same " + "regex. If this error is raised, it's a bug in snakefmt's " + "handling of snakemake syntax. Please report this to the " + "developers with the code so we can fix it: " + "https://github.com/snakemake/snakefmt/issues" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakefmt/formatter.py` around lines 127 - 133, Replace the runtime-conditional assert on re_rematch with an explicit exception to ensure the invariant is enforced even when Python is optimized; locate the assert re_rematch in formatter.py (the block that checks the regex match after formatting) and raise a clear exception type (e.g., RuntimeError or ValueError) with the same descriptive message about a bug in snakefmt's handling of snakemake syntax and the report URL so failures cannot be silently skipped under -O/-OO.
198-204: Same consideration:assertcan be disabled in optimized mode.This assertion guards an internal invariant. If running with
-O, a failed invariant would silently continue instead of alerting the user.💡 Suggested alternative
- assert not self.keywords, ( - "All directives should have been consumed; " - "if not, this is a bug in snakefmt's handling of snakemake syntax. " - "It must be the coder's fault, not the user's. " - "So please report this to the developers with the code so we can fix it: " - "https://github.com/snakemake/snakefmt/issues" - ) + if self.keywords: + raise RuntimeError( + "All directives should have been consumed; " + "if not, this is a bug in snakefmt's handling of snakemake syntax. " + "It must be the coder's fault, not the user's. " + "So please report this to the developers with the code so we can fix it: " + "https://github.com/snakemake/snakefmt/issues" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakefmt/formatter.py` around lines 198 - 204, The assert guarding that all directives were consumed (the assertion checking self.keywords) must not rely on Python's assert (which can be disabled with -O); replace the assert with an explicit runtime check that raises an AssertionError (or RuntimeError) with the same message when self.keywords is truthy. Locate the assertion involving self.keywords in snakefmt/formatter.py and change it to an if self.keywords: raise AssertionError("All directives should have been consumed; if not, this is a bug in snakefmt's handling of snakemake syntax. It must be the coder's fault, not the user's. So please report this to the developers with the code so we can fix it: https://github.com/snakemake/snakefmt/issues") so the invariant always fails loudly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@snakefmt/formatter.py`:
- Around line 127-133: Replace the runtime-conditional assert on re_rematch with
an explicit exception to ensure the invariant is enforced even when Python is
optimized; locate the assert re_rematch in formatter.py (the block that checks
the regex match after formatting) and raise a clear exception type (e.g.,
RuntimeError or ValueError) with the same descriptive message about a bug in
snakefmt's handling of snakemake syntax and the report URL so failures cannot be
silently skipped under -O/-OO.
- Around line 198-204: The assert guarding that all directives were consumed
(the assertion checking self.keywords) must not rely on Python's assert (which
can be disabled with -O); replace the assert with an explicit runtime check that
raises an AssertionError (or RuntimeError) with the same message when
self.keywords is truthy. Locate the assertion involving self.keywords in
snakefmt/formatter.py and change it to an if self.keywords: raise
AssertionError("All directives should have been consumed; if not, this is a bug
in snakefmt's handling of snakemake syntax. It must be the coder's fault, not
the user's. So please report this to the developers with the code so we can fix
it: https://github.com/snakemake/snakefmt/issues") so the invariant always fails
loudly.
In `@snakefmt/parser/parser.py`:
- Around line 49-64: The zip in split_token_lines can silently drop mismatched
sequences; update the return to use zip(..., strict=True) so
split_token_lines(token: tokenize.TokenInfo) raises if the range of line numbers
(range(token.start[0], token.end[0] + 1)) and
token.line.splitlines(keepends=True) lengths differ, preventing silent
mismatches during development.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c369a3db-6bf1-451b-a3d0-38f876d35d03
📒 Files selected for processing (2)
snakefmt/formatter.pysnakefmt/parser/parser.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_formatter.py (2)
2103-2110: Consider renaming unused loop variable to_formatted.The
formattedvariable is intentionally unused here because# fmt: offat the start means the originalcodeshould remain unformatted. Renaming to_formattedwould satisfy the linter and signal intent.- for code, formatted in ( + for code, _formatted in (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_formatter.py` around lines 2103 - 2110, In test_fmt_off_at_start rename the intentionally unused loop variable formatted to _formatted in the for loop (for code, _formatted in (...)) so the linter knows it’s unused; update the tuple unpack in the test_fmt_off_at_start function and leave all logic unchanged (references: test_fmt_off_at_start, variables code and formatted/_formatted).
2144-2156: Same unused variable pattern here.Same situation as
test_fmt_off_at_start— theformattedvariable is intentionally unused because the test verifies that formatting is disabled when# fmt: offis present.- for code, formatted in ( + for code, _formatted in (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_formatter.py` around lines 2144 - 2156, In test_fmt_off_not_on the loop unpacks into (code, formatted) but never uses formatted; change the unpack to use a throwaway name (e.g., for code, _ in ...) or iterate only over the first element (for code in ...) so the unused variable is removed; update the loop header in test_fmt_off_not_on to reference the throwaway "_" or iterate just over TestSimpleParamFormatting.example_* entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@snakefmt/parser/parser.py`:
- Around line 49-64: In split_token_lines, prevent silent truncation by calling
zip with strict=True so mismatched lengths between range(token.start[0],
token.end[0] + 1) and token.line.splitlines(keepends=True) raise instead of
dropping interior lines; update the zip invocation in split_token_lines to
include strict=True and preserve the existing use of
token.line.splitlines(keepends=True) so you can then handle or assert the
mismatch explicitly (this ensures split_token_lines will surface cases where
tokenize.TokenInfo.line does not contain all physical lines).
---
Nitpick comments:
In `@tests/test_formatter.py`:
- Around line 2103-2110: In test_fmt_off_at_start rename the intentionally
unused loop variable formatted to _formatted in the for loop (for code,
_formatted in (...)) so the linter knows it’s unused; update the tuple unpack in
the test_fmt_off_at_start function and leave all logic unchanged (references:
test_fmt_off_at_start, variables code and formatted/_formatted).
- Around line 2144-2156: In test_fmt_off_not_on the loop unpacks into (code,
formatted) but never uses formatted; change the unpack to use a throwaway name
(e.g., for code, _ in ...) or iterate only over the first element (for code in
...) so the unused variable is removed; update the loop header in
test_fmt_off_not_on to reference the throwaway "_" or iterate just over
TestSimpleParamFormatting.example_* entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b91bb32a-c9e4-4b93-9aa3-6e67cbe457a5
📒 Files selected for processing (4)
README.mdsnakefmt/formatter.pysnakefmt/parser/parser.pytests/test_formatter.py
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- snakefmt/formatter.py
mbhall88
requested changes
Mar 30, 2026
Member
mbhall88
left a comment
mbhall88
left a comment
There was a problem hiding this comment.
Thanks so much for tackling this @Hocnonsense! And great start.
I have added some comments mostly to the tests as these are the only part I really care about. There are some changes I don't like, such as extracting some of the test cases out of the relevant functions into variables outside the tests. I understand this has been done to be able to reuse them in later tests, but I would rather copy and past them into the later tests so that each test can be read completely in isolation and understand what is being tested and whether the formatting makes sense. I can't do this with the updated structure.
We should also support # fmt: skip to skip formatting a single line (see black's docs).
How does the indenting work in the example where the # fmt: on is indented closer to 0 than the previous # fmt: off? Or does the unindentation implicitly just turn the formatting back on, so the # fat: on has no effect? Might be nice to have a test for this.
There is intermittent test code cases where you aren't using explicit visual tab indication. e.g., f"{TAB * 1}..." etc. And also cases where you have f"{TAB}{TAB}.." should be changed to f"{TAB * 2}..."
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_formatter.py (1)
2097-2103: Unused loop variableformattedshould be renamed to_formatted.The loop unpacks
(code, formatted)but only usescode. Renaming the unused variable to_formattedfollows Python convention for intentionally unused variables and silences the Ruff B007 warning.- for code, formatted in ( + for code, _formatted in (This also applies to line 2153.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_formatter.py` around lines 2097 - 2103, Rename the unused loop variable formatted to _formatted in the test loops that unpack (code, formatted) — specifically around the uses of TestSimpleParamFormatting.example_shell_newline, TestSimpleParamFormatting.example_params_newline, TestSimpleParamFormatting.example_input_threads_newline and the second occurrence around line 2153 — so the for loop becomes for code, _formatted in (...) and satisfies the Ruff B007 convention; no other logic changes are needed, test helpers like setup_formatter() remain unchanged.snakefmt/formatter.py (1)
238-241: Redundantself.fmt_offcheck on line 240.The condition
if self.fmt_off and not self.result.endswith("\n\n\n")is inside anif self.fmt_off:block, making the innerself.fmt_offcheck redundant.Suggested fix
if self.fmt_off: if self.fmt_off[0] == 0 and not self.no_formatting_yet: - if self.fmt_off and not self.result.endswith("\n\n\n"): + if not self.result.endswith("\n\n\n"): self.result += "\n\n"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@snakefmt/formatter.py` around lines 238 - 241, The inner redundant check `if self.fmt_off` inside the outer `if self.fmt_off:` block should be removed: inside the method where you manipulate formatting state (the block referencing self.fmt_off, self.no_formatting_yet and self.result.endswith), simplify the nested condition so it only tests `if self.fmt_off[0] == 0 and not self.no_formatting_yet:` then `if not self.result.endswith("\n\n\n"):` before appending `"\n\n"`; update the condition references in that block (the ones using self.fmt_off, self.no_formatting_yet, and self.result.endswith) accordingly to eliminate the redundant check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@snakefmt/formatter.py`:
- Around line 238-241: The inner redundant check `if self.fmt_off` inside the
outer `if self.fmt_off:` block should be removed: inside the method where you
manipulate formatting state (the block referencing self.fmt_off,
self.no_formatting_yet and self.result.endswith), simplify the nested condition
so it only tests `if self.fmt_off[0] == 0 and not self.no_formatting_yet:` then
`if not self.result.endswith("\n\n\n"):` before appending `"\n\n"`; update the
condition references in that block (the ones using self.fmt_off,
self.no_formatting_yet, and self.result.endswith) accordingly to eliminate the
redundant check.
In `@tests/test_formatter.py`:
- Around line 2097-2103: Rename the unused loop variable formatted to _formatted
in the test loops that unpack (code, formatted) — specifically around the uses
of TestSimpleParamFormatting.example_shell_newline,
TestSimpleParamFormatting.example_params_newline,
TestSimpleParamFormatting.example_input_threads_newline and the second
occurrence around line 2153 — so the for loop becomes for code, _formatted in
(...) and satisfies the Ruff B007 convention; no other logic changes are needed,
test helpers like setup_formatter() remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 737314a3-8707-4aaa-93ed-615d62df92c3
📒 Files selected for processing (3)
snakefmt/formatter.pysnakefmt/parser/parser.pytests/test_formatter.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_formatter.py (1)
2369-2385:⚠️ Potential issue | 🟡 MinorPipeline failures: unused variables and f-strings without placeholders.
This test has several issues flagged by CI:
formatteris assigned but never used (line 2370)expectedis assigned but never used (line 2376)- f-strings without placeholders (lines 2372-2374)
The test assertion is commented out with a TODO, leaving dead code. Either complete the test implementation or remove the unreachable code to fix the pipeline.
🔧 Proposed fix: Either implement the test or mark it as skipped
Option 1 - Mark as skipped until implemented:
+ `@pytest.mark.skip`(reason="# fmt: skip in directives not yet supported") def test_fmt_skip_in_directive(self): - formatter = setup_formatter( + snakecode = ( "rule a:\n" - f" params:\n" - f" x = [ 1,2,3] # fmt: skip\n" - f" input: a= 'sth' # fmt: skip\n" + " params:\n" + " x = [ 1,2,3] # fmt: skip\n" + " input: a= 'sth' # fmt: skip\n" ) expected = ( "rule a:\n" f"{TAB}params:\n" f"{TAB * 2}x=[1, 2, 3], # fmt: skip\n" f"{TAB}input:\n" f'{TAB * 2}a="sth", # fmt: skip\n' ) - # TODO: currently `# fmt: skip` in directives is not supported - # assert formatter.get_formatted() == expected + # TODO: currently `# fmt: skip` in directives is not supported + # assert setup_formatter(snakecode).get_formatted() == expectedOption 2 - Remove the test entirely until the feature is implemented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_formatter.py` around lines 2369 - 2385, The test function test_fmt_skip_in_directive contains dead/unusable code: it creates formatter via setup_formatter, builds expected using f-strings without placeholders and leaves the assertion commented out, causing unused-variable and f-string lint errors; fix by either implementing the test properly (use non-f-string literals or put placeholders in those f-strings, call formatter.get_formatted() and assert equality with expected) or mark the whole test as skipped/removed: update test_fmt_skip_in_directive to call formatter.get_formatted() and assert against expected (ensuring TAB is a real variable or replace f-strings with plain strings), or replace the function with a pytest.skip() decorator or remove it completely to eliminate unused-variable and f-string issues.
🧹 Nitpick comments (2)
tests/test_formatter.py (2)
2096-2103: Unused loop variableformattedshould be renamed to_formatted.The loop unpacks tuples but only uses
code. Rename the unused variable to suppress the B007 warning.♻️ Proposed fix
def test_fmt_off_at_start(self): - for code, formatted in ( + for code, _formatted in ( TestSimpleParamFormatting.example_shell_newline, TestSimpleParamFormatting.example_params_newline, TestSimpleParamFormatting.example_input_threads_newline, ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_formatter.py` around lines 2096 - 2103, In test_fmt_off_at_start, the for-loop unpacks tuples into (code, formatted) but never uses formatted; rename the unused variable to _formatted to silence the B007 warning. Update the loop header in test_fmt_off_at_start to use (code, _formatted) when iterating over TestSimpleParamFormatting.example_shell_newline, TestSimpleParamFormatting.example_params_newline, and TestSimpleParamFormatting.example_input_threads_newline so the assert using setup_formatter(expected).get_formatted() remains unchanged.
2147-2164: Same issue: unused loop variableformatted.♻️ Proposed fix
def test_fmt_off_not_on(self): """` - `# fmt: on` at a deeper indentation level than `# fmt: off` has no effect - `# fmt: off` keeps the rest of the code unformatted until a same-indent `# fmt: on` found """ - for code, formatted in ( + for code, _formatted in ( TestSimpleParamFormatting.example_shell_newline, TestSimpleParamFormatting.example_params_newline, TestSimpleParamFormatting.example_input_threads_newline, ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_formatter.py` around lines 2147 - 2164, The test test_fmt_off_not_on declares an unused loop variable "formatted" by iterating with "for code, formatted in (TestSimpleParamFormatting.example_shell_newline, ...)" even though each tuple element is a single string; fix by iterating only over codes (use "for code in (TestSimpleParamFormatting.example_shell_newline, TestSimpleParamFormatting.example_params_newline, TestSimpleParamFormatting.example_input_threads_newline):") so the loop unpacks correctly and "formatted" is not required, ensuring setup_formatter(expected).get_formatted() compares as intended; update the loop in test_fmt_off_not_on and keep references to setup_formatter and the TestSimpleParamFormatting example attributes intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_formatter.py`:
- Around line 2369-2385: The test function test_fmt_skip_in_directive contains
dead/unusable code: it creates formatter via setup_formatter, builds expected
using f-strings without placeholders and leaves the assertion commented out,
causing unused-variable and f-string lint errors; fix by either implementing the
test properly (use non-f-string literals or put placeholders in those f-strings,
call formatter.get_formatted() and assert equality with expected) or mark the
whole test as skipped/removed: update test_fmt_skip_in_directive to call
formatter.get_formatted() and assert against expected (ensuring TAB is a real
variable or replace f-strings with plain strings), or replace the function with
a pytest.skip() decorator or remove it completely to eliminate unused-variable
and f-string issues.
---
Nitpick comments:
In `@tests/test_formatter.py`:
- Around line 2096-2103: In test_fmt_off_at_start, the for-loop unpacks tuples
into (code, formatted) but never uses formatted; rename the unused variable to
_formatted to silence the B007 warning. Update the loop header in
test_fmt_off_at_start to use (code, _formatted) when iterating over
TestSimpleParamFormatting.example_shell_newline,
TestSimpleParamFormatting.example_params_newline, and
TestSimpleParamFormatting.example_input_threads_newline so the assert using
setup_formatter(expected).get_formatted() remains unchanged.
- Around line 2147-2164: The test test_fmt_off_not_on declares an unused loop
variable "formatted" by iterating with "for code, formatted in
(TestSimpleParamFormatting.example_shell_newline, ...)" even though each tuple
element is a single string; fix by iterating only over codes (use "for code in
(TestSimpleParamFormatting.example_shell_newline,
TestSimpleParamFormatting.example_params_newline,
TestSimpleParamFormatting.example_input_threads_newline):") so the loop unpacks
correctly and "formatted" is not required, ensuring
setup_formatter(expected).get_formatted() compares as intended; update the loop
in test_fmt_off_not_on and keep references to setup_formatter and the
TestSimpleParamFormatting example attributes intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85a20e19-6853-4b35-88e5-647f90ed36af
📒 Files selected for processing (3)
README.mdsnakefmt/formatter.pytests/test_formatter.py
🚧 Files skipped from review as they are similar to previous changes (1)
- snakefmt/formatter.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_formatter.py`:
- Around line 2097-2101: The loop unpacks tuples like (code, formatted) using
TestSimpleParamFormatting.example_shell_newline,
TestSimpleParamFormatting.example_params_newline, and
TestSimpleParamFormatting.example_input_threads_newline but never uses the
formatted variable, causing Ruff B007; rename the unused variable to _formatted
(or _) in the for-loop header (e.g., change "for code, formatted in (...):" to
"for code, _formatted in (...):") in the loops at the occurrences around the
TestSimpleParamFormatting examples (also update the similar loop around lines
with the same examples at the second location).
- Around line 2369-2386: The test test_fmt_skip_in_directive does not actually
assert the formatted output against the expected string (it uses a no-op "assert
expected"), so update the assertion to compare the two: replace the current
assertions with a single assertion that formatter.get_formatted() == expected;
keep or update the TODO comment about unsupported `# fmt: skip` in directives if
desired, but ensure the primary check uses formatter.get_formatted() and
expected so the test fails on regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46995c87-d1b6-4406-a5f6-11769c16db04
📒 Files selected for processing (1)
tests/test_formatter.py
Comment on lines
+2097
to
+2101
| for code, formatted in ( | ||
| TestSimpleParamFormatting.example_shell_newline, | ||
| TestSimpleParamFormatting.example_params_newline, | ||
| TestSimpleParamFormatting.example_input_threads_newline, | ||
| ): |
There was a problem hiding this comment.
Unused loop variable formatted triggers Ruff B007 warnings.
formatted is unpacked but unused in these loops. Rename to _formatted (or _) to keep lint clean.
Suggested fix
- for code, formatted in (
+ for code, _formatted in (
TestSimpleParamFormatting.example_shell_newline,
TestSimpleParamFormatting.example_params_newline,
TestSimpleParamFormatting.example_input_threads_newline,
):
- for code, formatted in (
+ for code, _formatted in (
TestSimpleParamFormatting.example_shell_newline,
TestSimpleParamFormatting.example_params_newline,
TestSimpleParamFormatting.example_input_threads_newline,
):Also applies to: 2153-2157
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 2097-2097: Loop control variable formatted not used within loop body
Rename unused formatted to _formatted
(B007)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_formatter.py` around lines 2097 - 2101, The loop unpacks tuples
like (code, formatted) using TestSimpleParamFormatting.example_shell_newline,
TestSimpleParamFormatting.example_params_newline, and
TestSimpleParamFormatting.example_input_threads_newline but never uses the
formatted variable, causing Ruff B007; rename the unused variable to _formatted
(or _) in the for-loop header (e.g., change "for code, formatted in (...):" to
"for code, _formatted in (...):") in the loops at the occurrences around the
TestSimpleParamFormatting examples (also update the similar loop around lines
with the same examples at the second location).
Contributor
Author
|
@mbhall88 Thanks for the detailed review! Here is a summary of what has been addressed:
|
Member
|
Great work. Two things from me:
rule:
input:
"test.txt"
output:
"test.out"
params:
test_param="test",
long_param=lambda wildcards, input: "this is a really long line that should get formatted {}".format(input[0]) # fmt: skip
shell:
"echo {input} > {output}"gets formatted to rule:
input:
"test.txt",
output:
"test.out",
params:
test_param="test",
long_param=lambda wildcards, input: "this is a really long line that should get formatted {}".format(
input[0]
), # fmt: skip
shell:
"echo {input} > {output}"I would expect that long line to remain as it is.
and if they can be inline, do they apple to the line they are on, or just subsequent lines? |
Contributor
Author
|
@mbhall88 Thanks for reviewing!
|
mbhall88
requested changes
Apr 1, 2026
Member
Ah okay, I see. It would be nice to have the full support straight away, but I am happy to merge this PR for now, and then we work on adding skip before the next release? I had hoped we would be able to just pass the comment straight through to black for skip as it is a little more manageable to do it inline rather than the more complex block commenting
Okay, good to know. Might be good to make this explicit in the README |
Contributor
Author
|
Thanks! Happy to merge now, and I'm happy to follow up with full directive-level Regarding passing it through to Black: the challenge is that parameter lines inside rules go through |
mbhall88
approved these changes
Apr 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Format Directives
snakefmtsupports inline comment directives to control formatting behaviour for specific regions of code.# fmt: off/# fmt: onDisables all formatting for the region between the two directives. The directives must appear at the same indentation level. A
# fmt: onat a deeper indent than the matching# fmt: offhas no effect.Note: inside
run:blocks and other Python code,# fmt: off/# fmt: onis passed through to [Black][black] which handles it natively.# fmt: off[sort]Disables only directive sorting for the region, while still applying all other formatting. Useful when you want to preserve a custom directive order for a specific rule.
A plain
# fmt: on(without[sort]) also ends a# fmt: off[sort]region.# fmt: off[next]Disables formatting for the single next Snakemake keyword block (e.g.
rule,checkpoint,use rule). Only that one block is left unformatted; subsequent blocks are formatted normally.Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests