feat: add cljfmt :partial mode to format only replaced forms (#154)#155
Open
williamp44 wants to merge 1 commit intobhauman:mainfrom
Open
feat: add cljfmt :partial mode to format only replaced forms (#154)#155williamp44 wants to merge 1 commit intobhauman:mainfrom
williamp44 wants to merge 1 commit intobhauman:mainfrom
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 a cljfmt ":partial" mode to format only the edited form in isolation, updates docs, config, and schema to accept Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Pipeline
participant Finder
participant Pos
participant Formatter
participant FileFormatter
participant Editor
Client->>Pipeline: request edit (replace/insert)
Pipeline->>Finder: parse-source & locate form
Finder-->>Pipeline: positioned zipper / located form
Pipeline->>Pos: capture-form-position (compute ::form-col)
Pos-->>Pipeline: ::form-col (maybe nil)
alt cljfmt = :partial and ::form-col present
Pipeline->>Formatter: format-new-source-partial (::new-source-code, ::form-col)
Formatter->>Formatter: format-form-in-isolation and re-indent-to-column
Formatter-->>Pipeline: updated ::new-source-code
Pipeline->>Editor: edit-form (insert formatted form)
Editor-->>Pipeline: source with replacement
Pipeline->>FileFormatter: format-source (skip whole-file formatting)
FileFormatter-->>Pipeline: unchanged surrounding formatting
else
Pipeline->>Editor: edit-form (no pre-format)
Editor-->>Pipeline: source with replacement
Pipeline->>FileFormatter: format-source (apply cljfmt to entire file)
FileFormatter->>FileFormatter: apply cljfmt to whole file
FileFormatter-->>Pipeline: fully formatted source
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/clojure_mcp/tools/form_edit/pipeline_test.clj (1)
421-453: Move temp-dir cleanup intofinallyblocks for failure-safe teardown.Several tests clean temp dirs only at the end of happy-path execution. If an assertion fails earlier, temp artifacts can leak and affect later runs.
Pattern to apply
- (let [test-dir (test-utils/create-test-dir) - ...] - ... - (test-utils/clean-test-dir test-dir)) + (let [test-dir (test-utils/create-test-dir)] + (try + (let [...] + ...) + (finally + (test-utils/clean-test-dir test-dir))))Also applies to: 460-484, 525-553, 560-588
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/clojure_mcp/tools/form_edit/pipeline_test.clj` around lines 421 - 453, The test creates a temp test-dir via test-utils/create-test-dir and currently calls test-utils/clean-test-dir only at the end of the happy path; wrap the body that uses test-dir (the let block that calls sut/edit-form-pipeline, checks result, and reads file-path) in a try...finally and move the test-utils/clean-test-dir call into the finally so cleanup always runs; apply the same change to the other similar tests referenced (the blocks around lines 460-484, 525-553, 560-588) to ensure temp dirs are removed on failures as well.src/clojure_mcp/tools/form_edit/pipeline.clj (1)
243-245: Don’t default missing position to column 1 in partial mode.If
z/positionis unavailable, setting::form-colto1still triggers the “already preformatted” skip path, which can suppress full-file fallback and mis-indent nested edits.Proposed fix
(defn capture-form-position @@ [ctx] (let [zloc (::zloc ctx)] (if-let [pos (z/position zloc)] (assoc ctx ::form-col (second pos)) ;; [row col] -> col - (assoc ctx ::form-col 1)))) ;; default to column 1 + ctx))) ;; leave unset so downstream can fall back (defn format-new-source-partial @@ (let [nrepl-client-map (some-> ctx ::nrepl-client-atom deref) cljfmt-setting (config/get-cljfmt nrepl-client-map)] - (if (= :partial cljfmt-setting) + (if (and (= :partial cljfmt-setting) (some? (::form-col ctx))) (let [new-source (::new-source-code ctx) - target-col (or (::form-col ctx) 1) + target-col (::form-col ctx) formatting-options (core/project-formatting-options nrepl-client-map) formatted (core/format-form-in-isolation new-source target-col formatting-options)] (assoc ctx ::new-source-code formatted)) ;; Not :partial mode, pass through ctx)))Also applies to: 257-263, 320-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/clojure_mcp/tools/form_edit/pipeline.clj` around lines 243 - 245, The code currently defaults ::form-col to 1 when z/position returns nil, which incorrectly signals a column and triggers the “already preformatted” skip; change the logic so you only assoc ::form-col when (z/position zloc) yields a value (i.e., keep ctx unchanged when pos is nil) instead of associating 1, and apply the same fix at the other occurrences that default to 1 (the blocks around the other z/position uses you noted). In short: remove the fallback assoc of ::form-col 1, only assoc ::form-col (second pos) when pos exists, and ensure downstream code relies on presence/absence of ::form-col rather than a sentinel value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/clojure_mcp/tools/form_edit/pipeline.clj`:
- Around line 243-245: The code currently defaults ::form-col to 1 when
z/position returns nil, which incorrectly signals a column and triggers the
“already preformatted” skip; change the logic so you only assoc ::form-col when
(z/position zloc) yields a value (i.e., keep ctx unchanged when pos is nil)
instead of associating 1, and apply the same fix at the other occurrences that
default to 1 (the blocks around the other z/position uses you noted). In short:
remove the fallback assoc of ::form-col 1, only assoc ::form-col (second pos)
when pos exists, and ensure downstream code relies on presence/absence of
::form-col rather than a sentinel value.
In `@test/clojure_mcp/tools/form_edit/pipeline_test.clj`:
- Around line 421-453: The test creates a temp test-dir via
test-utils/create-test-dir and currently calls test-utils/clean-test-dir only at
the end of the happy path; wrap the body that uses test-dir (the let block that
calls sut/edit-form-pipeline, checks result, and reads file-path) in a
try...finally and move the test-utils/clean-test-dir call into the finally so
cleanup always runs; apply the same change to the other similar tests referenced
(the blocks around lines 460-484, 525-553, 560-588) to ensure temp dirs are
removed on failures as well.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CONFIG.mdPROJECT_SUMMARY.mdsrc/clojure_mcp/config.cljsrc/clojure_mcp/config/schema.cljsrc/clojure_mcp/tools/form_edit/core.cljsrc/clojure_mcp/tools/form_edit/pipeline.cljtest/clojure_mcp/config/schema_test.cljtest/clojure_mcp/tools/form_edit/pipeline_test.clj
williamp44
force-pushed
the
fix/cljfmt-partial-formatting
branch
from
afaf190 to
23e41e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/clojure_mcp/tools/form_edit/pipeline_test.clj (1)
421-452: Wrap temp-dir cleanup infinallyto avoid test pollution on assertion failure.A few tests clean temp dirs only on the success path. If an assertion fails early, cleanup is skipped.
Also applies to: 460-484, 491-518, 525-553, 560-588, 643-675, 690-711
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/clojure_mcp/tools/form_edit/pipeline_test.clj` around lines 421 - 452, Tests create a temp dir via test-utils/create-test-dir and only call test-utils/clean-test-dir on the success path, so wrap the cleanup in a finally block to guarantee deletion even if an assertion fails: surround the setup, pipeline execution (sut/edit-form-pipeline and sut/format-result) and assertions with a try (or try/finally) and move the call to test-utils/clean-test-dir test-dir into the finally clause; apply the same change to the other test blocks that currently call test-utils/clean-test-dir only on the success path (the other blocks using test-dir, create-and-register-test-file, sut/edit-form-pipeline, sut/format-result).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/clojure_mcp/tools/form_edit/pipeline.clj`:
- Around line 243-245: The code currently defaults ::form-col to 1 when
z/position returns nil, which causes silent mis-indentation in :partial mode;
change the logic in the block that uses z/position and zloc so that if
z/position is nil and the operation-mode or context indicates :partial you do
not set ::form-col to 1 (leave ::form-col absent or nil on ctx) so the caller
can trigger the full-file fallback; apply the same conditional change for the
similar block around the 257-264 region—only apply the column=1 default when not
in :partial mode and explicitly set or omit ::form-col accordingly.
---
Nitpick comments:
In `@test/clojure_mcp/tools/form_edit/pipeline_test.clj`:
- Around line 421-452: Tests create a temp dir via test-utils/create-test-dir
and only call test-utils/clean-test-dir on the success path, so wrap the cleanup
in a finally block to guarantee deletion even if an assertion fails: surround
the setup, pipeline execution (sut/edit-form-pipeline and sut/format-result) and
assertions with a try (or try/finally) and move the call to
test-utils/clean-test-dir test-dir into the finally clause; apply the same
change to the other test blocks that currently call test-utils/clean-test-dir
only on the success path (the other blocks using test-dir,
create-and-register-test-file, sut/edit-form-pipeline, sut/format-result).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CONFIG.mdPROJECT_SUMMARY.mdsrc/clojure_mcp/config.cljsrc/clojure_mcp/config/schema.cljsrc/clojure_mcp/tools/form_edit/core.cljsrc/clojure_mcp/tools/form_edit/pipeline.cljtest/clojure_mcp/config/schema_test.cljtest/clojure_mcp/tools/form_edit/pipeline_test.clj
🚧 Files skipped from review as they are similar to previous changes (3)
- test/clojure_mcp/config/schema_test.clj
- src/clojure_mcp/config/schema.clj
- src/clojure_mcp/config.clj
williamp44
force-pushed
the
fix/cljfmt-partial-formatting
branch
from
23e41e1 to
6a6a442
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/clojure_mcp/tools/form_edit/pipeline.clj (1)
243-264:⚠️ Potential issue | 🟠 MajorDon’t coerce missing form position to column
1in:partialmode.At Line 245, setting
::form-colto1when position is unavailable causes:partialflow to proceed as if tracking succeeded. Combined with Line 257 and Line 320, this can mis-indent edits and incorrectly skip full-file fallback.Suggested fix
(defn capture-form-position @@ [ctx] (let [zloc (::zloc ctx)] (if-let [pos (z/position zloc)] (assoc ctx ::form-col (second pos)) ;; [row col] -> col - (assoc ctx ::form-col 1)))) ;; default to column 1 + ctx))) ;; keep nil/absent so downstream can fallback (defn format-new-source-partial @@ (let [nrepl-client-map (some-> ctx ::nrepl-client-atom deref) cljfmt-setting (config/get-cljfmt nrepl-client-map)] - (if (= :partial cljfmt-setting) + (if (and (= :partial cljfmt-setting) (::form-col ctx)) (let [new-source (::new-source-code ctx) - target-col (or (::form-col ctx) 1) + target-col (::form-col ctx) formatting-options (core/project-formatting-options nrepl-client-map) formatted (core/format-form-in-isolation new-source target-col formatting-options)] (assoc ctx ::new-source-code formatted)) ;; Not :partial mode, pass through ctx)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/clojure_mcp/tools/form_edit/pipeline.clj` around lines 243 - 264, The code currently defaults ::form-col to 1 and then always runs partial formatting; instead, in format-new-source-partial only perform partial formatting when the position is actually present: check (contains? ctx ::form-col) or that (::form-col ctx) is non-nil before calling core/format-form-in-isolation, and if the form column is missing return ctx unchanged (so downstream code can fall back to full-file formatting). Update format-new-source-partial to gate the :partial branch on the presence of ::form-col (referencing ::new-source-code, ::form-col, core/format-form-in-isolation and config/get-cljfmt) rather than coercing a default column.
🧹 Nitpick comments (1)
test/clojure_mcp/tools/form_edit/pipeline_test.clj (1)
366-393: Add a regression test for “position unavailable” in partial mode.Current coverage validates known columns, but not the
z/position -> nilpath that should trigger fallback behavior instead of treating it as column 1.Suggested test case
(deftest capture-form-position-test @@ (is (= 9 (::sut/form-col result)) "Nested form should be at column 9")))) + +(deftest capture-form-position-missing-position-test + (testing "capture-form-position leaves ::form-col absent when position tracking is unavailable" + (with-redefs [z/position (fn [_] nil)] + (let [ctx {::sut/zloc :dummy-zloc} + result (sut/capture-form-position ctx)] + (is (nil? (::sut/form-col result)))))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/clojure_mcp/tools/form_edit/pipeline_test.clj` around lines 366 - 393, Add a regression test in capture-form-position-test that simulates the “position unavailable” (z/position -> nil) scenario in partial mode and asserts the fallback behavior instead of treating the form as column 1: call sut/parse-source and sut/find-form as other tests do but set the context to partial mode (include ::sut/partial true or whatever flag your parser uses to indicate partial parsing), ensure the found node yields a nil position from z/position, then call sut/capture-form-position and assert the returned ::sut/form-col matches the expected fallback value (or that a specific fallback path/flag is set) and not 1; reference capture-form-position, find-form, parse-source, ::sut/partial, and z/position when locating code to exercise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/clojure_mcp/tools/form_edit/pipeline.clj`:
- Around line 243-264: The code currently defaults ::form-col to 1 and then
always runs partial formatting; instead, in format-new-source-partial only
perform partial formatting when the position is actually present: check
(contains? ctx ::form-col) or that (::form-col ctx) is non-nil before calling
core/format-form-in-isolation, and if the form column is missing return ctx
unchanged (so downstream code can fall back to full-file formatting). Update
format-new-source-partial to gate the :partial branch on the presence of
::form-col (referencing ::new-source-code, ::form-col,
core/format-form-in-isolation and config/get-cljfmt) rather than coercing a
default column.
---
Nitpick comments:
In `@test/clojure_mcp/tools/form_edit/pipeline_test.clj`:
- Around line 366-393: Add a regression test in capture-form-position-test that
simulates the “position unavailable” (z/position -> nil) scenario in partial
mode and asserts the fallback behavior instead of treating the form as column 1:
call sut/parse-source and sut/find-form as other tests do but set the context to
partial mode (include ::sut/partial true or whatever flag your parser uses to
indicate partial parsing), ensure the found node yields a nil position from
z/position, then call sut/capture-form-position and assert the returned
::sut/form-col matches the expected fallback value (or that a specific fallback
path/flag is set) and not 1; reference capture-form-position, find-form,
parse-source, ::sut/partial, and z/position when locating code to exercise.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CONFIG.mdPROJECT_SUMMARY.mdsrc/clojure_mcp/config.cljsrc/clojure_mcp/config/schema.cljsrc/clojure_mcp/tools/form_edit/core.cljsrc/clojure_mcp/tools/form_edit/pipeline.cljtest/clojure_mcp/config/schema_test.cljtest/clojure_mcp/tools/form_edit/pipeline_test.clj
🚧 Files skipped from review as they are similar to previous changes (4)
- src/clojure_mcp/config.clj
- PROJECT_SUMMARY.md
- test/clojure_mcp/config/schema_test.clj
- CONFIG.md
williamp44
force-pushed
the
fix/cljfmt-partial-formatting
branch
2 times, most recently
from
3dff6ef to
0c92c83
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/clojure_mcp/tools/form_edit/pipeline.clj (1)
45-45: Consider tightening::form-coltopos-int?.Columns are 1-based; using
int?allows invalid0/negative values into context state.Proposed spec tightening
-(s/def ::form-col int?) +(s/def ::form-col pos-int?)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/clojure_mcp/tools/form_edit/pipeline.clj` at line 45, The ::form-col spec currently allows any integer (int?), letting 0 and negatives through; change its predicate to pos-int? so only positive 1-based column numbers are valid. Locate the declaration (s/def ::form-col int?) in pipeline.clj and replace int? with pos-int?, then run/update any callers or tests that construct context state to ensure they supply positive integers (or add validation/conversion before constructing the spec if needed).test/clojure_mcp/tools/form_edit/pipeline_test.clj (1)
441-739: Consider extracting temp-file lifecycle helpers for these integration tests.There’s repeated create/set/cleanup logic; a shared helper (or fixture wrapper) would reduce duplication and make cleanup more reliable when assertions fail mid-test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/clojure_mcp/tools/form_edit/pipeline_test.clj` around lines 441 - 739, The tests repeat the same temp-file lifecycle (test-utils/create-test-dir, test-utils/create-and-register-test-file, config/set-config!, and test-utils/clean-test-dir with try/finally) across many cases; extract a reusable helper (e.g., with-temp-test-file or fixture wrapper) that (1) creates the test-dir and registers the file, (2) yields the file-path and any teardown/restore actions to the test body, and (3) guarantees cleanup and config reset on success or failure so each test (those invoking sut/edit-form-pipeline like edit-form-pipeline-partial-formatting-test, issue-154-e2e-test, sexp-edit-pipeline-partial-fallback-test, etc.) can call the helper to reduce duplication and ensure reliable cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/clojure_mcp/tools/form_edit/pipeline.clj`:
- Line 45: The ::form-col spec currently allows any integer (int?), letting 0
and negatives through; change its predicate to pos-int? so only positive 1-based
column numbers are valid. Locate the declaration (s/def ::form-col int?) in
pipeline.clj and replace int? with pos-int?, then run/update any callers or
tests that construct context state to ensure they supply positive integers (or
add validation/conversion before constructing the spec if needed).
In `@test/clojure_mcp/tools/form_edit/pipeline_test.clj`:
- Around line 441-739: The tests repeat the same temp-file lifecycle
(test-utils/create-test-dir, test-utils/create-and-register-test-file,
config/set-config!, and test-utils/clean-test-dir with try/finally) across many
cases; extract a reusable helper (e.g., with-temp-test-file or fixture wrapper)
that (1) creates the test-dir and registers the file, (2) yields the file-path
and any teardown/restore actions to the test body, and (3) guarantees cleanup
and config reset on success or failure so each test (those invoking
sut/edit-form-pipeline like edit-form-pipeline-partial-formatting-test,
issue-154-e2e-test, sexp-edit-pipeline-partial-fallback-test, etc.) can call the
helper to reduce duplication and ensure reliable cleanup.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CONFIG.mdPROJECT_SUMMARY.mdsrc/clojure_mcp/config.cljsrc/clojure_mcp/config/schema.cljsrc/clojure_mcp/tools/form_edit/core.cljsrc/clojure_mcp/tools/form_edit/pipeline.cljtest/clojure_mcp/config/schema_test.cljtest/clojure_mcp/tools/form_edit/pipeline_test.clj
🚧 Files skipped from review as they are similar to previous changes (4)
- PROJECT_SUMMARY.md
- src/clojure_mcp/tools/form_edit/core.clj
- CONFIG.md
- src/clojure_mcp/config.clj
…#154) When :cljfmt is set to :partial in config, clojure_edit formats only the replacement form in isolation (via cljfmt) and re-indents it to the correct column position, rather than reformatting the entire file. This prevents collateral formatting changes to unrelated code. - Add re-indent-to-column and format-form-in-isolation pure functions - Add capture-form-position and format-new-source-partial pipeline steps - Update format-source to skip whole-file formatting when partial pre-formatting ran - sexp-edit-pipeline falls back to full-file formatting in :partial mode - Update schema to accept [:or :boolean [:= :partial]] - 298 tests, 2153 assertions, 0 failures
williamp44
force-pushed
the
fix/cljfmt-partial-formatting
branch
from
0c92c83 to
232aea9
Compare
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
1 participant
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.
Summary
:partialmode for:cljfmtconfig that formats only the replaced/inserted form in isolation, preserving surrounding file formattingclojure_editreplace reformats entire file via cljfmt, not just the target form causing Noisy diffs and Stale REPL vars #154 —clojure_editreplace was reformatting the entire file via cljfmt, causing collateral whitespace changes to unrelated codeApproach
Per your guidance in #154: format the replacement content in isolation, then re-indent to match the target column position.
re-indent-to-columnadjusts indentation for the target positionformat-sourceskips whole-file formatting (the form is already formatted):partialmode (no position tracking for arbitrary s-expression matching)Changes
config/schema.clj—:cljfmtaccepts[:or :boolean [:= :partial]]config.clj—process-configpreserves:partial;get-cljfmtdocuments 3 valuesform_edit/core.clj— Two new pure functions:re-indent-to-column,format-form-in-isolationform_edit/pipeline.clj— Two new pipeline steps:capture-form-position,format-new-source-partial;format-sourceupdated to 3-way condCONFIG.md/PROJECT_SUMMARY.md— DocumentationConfig usage
{:cljfmt :partial} ;; format only the edited form {:cljfmt true} ;; full-file formatting (default, unchanged) {:cljfmt false} ;; no formatting (unchanged)Test plan
re-indent-to-column(col 1, col > 1, blank lines, single-line)format-form-in-isolation(success + failure fallback)capture-form-positiontested at col 1 and col 9 (nested reader conditional):partial+:replace— target formatted, surrounding(ns test.core)preserved:partial+:beforeand:after— inserted form formatted, surrounding preservedtruemode contrast — confirms(ns test.core)gets normalized:partial— falls back to full-file formattingprocess-configround-trip —:partialsurvives EDN → config processingSummary by CodeRabbit
New Features
:partialcljfmt mode that formats only the edited form, preserving surrounding formatting.Behavior
:partial, skipping full-file reformat when applicable;trueremains full-file,falsedisables formatting. Partial edits preserve alignment and fallback safely on failure.Documentation
true(full-file),:partial(form-only), andfalse(disabled).Tests