[sirmspencer]

Why

:split-keypairs-over-multiple-lines was a boolean: either every map got one
keypair per line, or none did. That is too coarse in practice. A map written
on a single line is usually deliberate and reads fine, whereas a map that
already spans several lines is where inconsistent keypair placement actually
costs readability. A project wanting the rule only for those cases had no way
to express it.

Approach

The option now takes a mode rather than a boolean:

Value	Applies to
:all	any map
:only-multiline	any map that spans more than one line
:none	nothing, the default

Splitting is now decided per map rather than per map entry, since
:only-multiline needs a decision about the map as a whole before any of its
keys are considered.

Performance

split-map-keypairs looks more complex than the insert-newline-left it
replaces, because it absorbs work that used to live in the predicate beside it.
That predicate identified keys with index-of, which counts by walking left
from every element it is called on, so the scan grew with the square of the
keypair count. Inserting the newline was never the expensive part.

Times to split a single map under :all, comparing the index based scan
against this one. Measured on the JVM, one timed run after two warmup passes:

Keypairs	Index based scan	This PR
50	9.3 ms	1.3 ms
100	28.1 ms	1.5 ms
200	87.7 ms	1.9 ms
400	295.9 ms	3.3 ms
800	1183.2 ms	5.5 ms

Compatibility

:split-keypairs-over-multiple-lines? is deprecated. It continues to work,
mapping to :all, and emits a deprecation warning.

The command line flag is unchanged for existing callers. Passing it with no
argument still means :all, and --no-split-keypairs-over-multiple-lines
still disables the rule. It additionally accepts a mode, given either as
--split-keypairs-over-multiple-lines only-multiline or
--split-keypairs-over-multiple-lines=only-multiline.

[weavejester]

Thank you for the PR.

My first impression is that the :mixed and :multiline options aren't immediately understandable. I can see a use-case for having different ways of formatting maps, but I wonder if it needs to be more deterministic? Like perhaps based on length or something.

[sirmspencer]

I can think on the names. There might not be need to mixed and multiline to be two things. multiline is for the common case that I see where destructuring, especially in a function signature will but the keys on separate lines, and then sometimes the :as is right after the closing ]. this looks odd once you add column alignment rules.

(defn my-function
  "doc string"
  [{:keys [a
           bbbbbbbbbbbbbb
           c]                       :as params}]
  ...)

It still skips the rest of the maps. So if you are manually making intentional multi line maps like in 2 columns they stay.

(def x
  {:a 1 :b 2
   :c 3 :d 4})

I can kind of see how multiline might make sense looking at the function signature, but if you are doing intentional multi line whatever somewhere else, it wouldn't be so obvious to cljfmt if it really should split the maps on new lines.

Our team is going to use mixed, so I'm fine dropping multiline if its more clear (and probably use the "multiline" name there). It's then always "if there is any line breaks in the map, add line breaks for the rest of the map". Does that feel more deterministic?

Using length is an option, but i think that fits more in the broader discussion on code width rules. We could certainly add a max[n] rule for that. Its not the problem I am solving for in this pr, which is I don't like maps with mixed new lines and not.

[sirmspencer]

I went ahead and took my own suggestion. The (now) single new mode should be much more clear on its intention.

[weavejester]

Could you give an example of how this change to the setting affects various different keypair-like structures?

[sirmspencer]

Could you give an example of how this change to the setting affects various different keypair-like structures?

Did you see the included examples file? https://github.com/weavejester/cljfmt/pull/430/changes#diff-f1452dc72ce6d80c4028b230a90a323659918b0102235c4808075679951673e9R1

[weavejester]

Ah, no, I didn't. Let's use the following keywords:

• :all instead of :always
• :none as a synonym for nil
• :only-multiline instead of :multiline.

Next, use the existing project file structure. So put the tests in the core tests. Move the docs to the base docs directory, and call it SPLIT-KEYPAIRS.md.

[sirmspencer]

I can use those names.

I was assuming you would have questions on the file splits. The core_test file is 3500 lines of code, its getting hard to find tests in.

[weavejester]

Splitting up core-test isn't unreasonable, but we should limit that to another PR. For some reason GraalVM is no longer downloading in the CI script, so let me see if the URL was moved or something.

edit: I see you've already investigated the broken CI and pushed a fix. I've merged that PR. Thanks!

[weavejester]

Since keypair-split-mode already converts true to :all, could we write this as:

(or (= mode :all)
    (and (= mode :only-multiline) (multiline? zloc)))

[sirmspencer]

yes, sorry, i usually catch these.