Skip to content

Fix single-column-line? behavior for last elements#411

Merged
weavejester merged 1 commit into
weavejester:masterfrom
beatriz:fix-align-single-column-lines
Apr 13, 2026
Merged

Fix single-column-line? behavior for last elements#411
weavejester merged 1 commit into
weavejester:masterfrom
beatriz:fix-align-single-column-lines

Conversation

@beatriz
Copy link
Copy Markdown
Contributor

@beatriz beatriz commented Apr 1, 2026

I noticed that the :single-column-line? rule is not currently respected when the single-column element is the last one in a map. This PR fixes that.

Comment thread cljfmt/src/cljfmt/core.cljc Outdated
Comment on lines +634 to +637
(let [right* (z/right* zloc)]
(and (or (nil? right*)
(line-break? (skip-whitespace-and-commas right* z/right*)))
(line-break? (skip-whitespace-and-commas (z/left* zloc) z/left*)))))
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please correct me if I'm wrong, but wouldn't we want to check for nil after skipping the whitespace and commas? i.e.

  (and (let [zloc (skip-whitespace-and-commas (z/right* zloc) z/right*)]
         (or (nil? zloc) (line-break? zloc)))
       (line-break? (skip-whitespace-and-commas (z/left* zloc) z/left*)))

Otherwise a single column line ending in a comma or whitespace wouldn't trigger this rule.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are correct! Fixed it.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Could you use the code I suggested in my previous comment? It makes it a little more concise and constrains the scope of the let clause.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ready to review, when you have the chance 🙂

Comment thread cljfmt/test/cljfmt/core_test.cljc Outdated
Comment on lines 2719 to 2729
["{:key1 1"
" :key3 3"
" :key2"
" (fn [a b c d e]"
" (+ a b c d e))}"]
["{:key1 1"
" :key3 3"
" :key2"
" (fn [a b c d e]"
" (+ a b c d e))}"]
{:align-map-columns? true})))
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we should have some invalid indentation so that there's a change in the two expression strings, otherwise we can't distinguish between it working and it doing nothing. So perhaps:

    (is (reformats-to?
         ["{:key-first 1"
          " :key2 3"
          " :key3"
          " (fn [a b c d e]"
          "   (+ a b c d e))}"]
         ["{:key-first 1"
          " :key2      3"
          " :key3"
          " (fn [a b c d e]"
          "   (+ a b c d e))}"]
         {:align-map-columns? true})))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes total sense, I've adjusted the test.

@beatriz beatriz requested a review from weavejester April 2, 2026 08:31
@weavejester
Copy link
Copy Markdown
Owner

This all looks good! Can you squash down your commits and give them an appropriate commit message like:

Fix single-column-line? behavior for last elements

@beatriz beatriz force-pushed the fix-align-single-column-lines branch from fa176c9 to 17af2e3 Compare April 13, 2026 09:26
@beatriz
Copy link
Copy Markdown
Contributor Author

beatriz commented Apr 13, 2026

Of course, done!

@weavejester weavejester merged commit c5fb9d4 into weavejester:master Apr 13, 2026
1 check passed
@weavejester
Copy link
Copy Markdown
Owner

Merged. Thanks!

@beatriz
Copy link
Copy Markdown
Contributor Author

beatriz commented Apr 13, 2026

Cool! Do you have a release planned for soon?

@weavejester
Copy link
Copy Markdown
Owner

Just doing it right now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants