Add optional column-oriented wrapping via :max-line-length

[ispandey81]

Refs #114

Apply :max-line-length as a structural pass: break only between direct children of eligible single-line forms using existing width accounting. Cover lists (including ns :import / :use), vectors, maps, sets, fn literals, namespaced maps, and inner collections in #? / #?@ reader conditionals; skip layout changes inside other reader macros and inside ns :require libspec vectors (row breaks only between those vectors). When :max-line-length is set, gate map keypair newlines on line width; keep unconditional keypair splitting when only
:split-keypairs-over-multiple-lines? is enabled. Document behavior and split/max interaction in README; extend core tests accordingly.

[ispandey81]

Hi @weavejester , this is my first stab at #114
Let me know if this is a step in the right direction.

[weavejester]

Can you explain why the split-keypairs-over-multiple-lines function needs to be changed in this way? I think there needs to be a very good reason to add complexity here.

[ispandey81]

If we keep the old behavior when both are on, the keypair pass always explodes maps onto many lines before the line-length pass runs. That makes :max-line-length almost meaningless for maps (it all stems from my approach being dependent on the outer form which I agree is not generic)

My intent with this change: with a positive max and keypair, breaks on maps are width-gated; split-only (no max) stays exactly the old “always split qualifying keys” behavior.

[weavejester]

Can you explain why there's a separate function for libspecs? Why is breaking the lines here different to other forms?

[ispandey81]

General structural wrap inserts newlines between direct children of lists/vectors. Breaking “at the first child boundary” inside [my.ns :as m] splits the same require entry across lines, which is not ideal. If we later get configurable break rules (#413), this could become one configured rule among others.

[weavejester]

I think we want to attempt this feature in two steps:

1. Get a basic line-breaking feature
2. Create some sort of per-form syntax for preferring line break locations

I don't think we should try to hard code specific behavior, as (a) we'll have to eventually remove it in favor of a configurable system, and (b) we don't want to break things when we switch over systems.

[weavejester]

Why was this change made?

[ispandey81]

my bad, this should be reset to the way it was.. Initial drafts of my change were reporting an error for this when I ran bb lint

[weavejester]

I think we should be aiming for a function like:

(defn limit-line-length [form {:keys [max-line-length]}]
  (transform edit-all #(needs-line-break? % max-line-length) add-line-break))

[ispandey81]

I will take this feedback and apply it for my next iteration of this PR

[weavejester]

This is looks like the logic for line breaks depends on the form, but why is that the case? Why not add line breaks at the first possible place prior to the max-line-length?

If certain forms are going to need specific line break, that sounds like something that should be configurable (see #413).

[ispandey81]

My thinking was hard wrap at max-line-length is going to be unsafe e.g.
very-long-namespace/foo or keyword-with-dashes must stay intact.
String literals cannot be broken without changing semantics etc

I will think about not being dependent on the form and make this more generic

[weavejester]

By "the first possible place" I mean the first possible place that doesn't change the semantic meaning.

[weavejester]

Thanks for the PR, I have a few comments which I've made inline.

Generally I think this needs to be simpler and more general. Where the line break happens should be very predictable, and shouldn't depend on what the outer form is.

I also note that the PR doesn't seem to include calls to the indentation functions, unless I've missed that somehow? You need to know how a line will be indented in order to know whether a line break is in an appropriate place. For instance:

;; too long a line
(aaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb cccccccccccccccccccccccccccccccccccccccccccccccccccccccc)

;; breaking it up here also leads to too long a line due to indent
(aaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
                              cccccccccccccccccccccccccccccccccccccccccccccccccccccccc)


;; so it needs to be split like so:
(aaaaaaaaaaaaaaaaaaaaaaaaaaaa
 bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
 cccccccccccccccccccccccccccccccccccccccccccccccccccccccc)

;; however, if the indentation rules were different, this might be valid:
(aaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
  cccccccccccccccccccccccccccccccccccccccccccccccccccccccc)

[weavejester]

I think before any more code is written, we need to define the algorithm we're using to divide up lines. This needs to:

1. Work with lines of arbitrary length
2. Be able to split a line up more than once if necessary
3. Take into account indentation
4. Accept that some lines cannot be split up without user intervention (e.g. long string literals)

Splitting one line into two seems relatively straightforward: we start from the line break and step left until we get a line break position that works. However, splitting one line into three or more seems more complex. Consider a situation where we wrap at 10 characters:

;123456789
(aa bb (cc dd ee ff))

How should this be wrapped? Presumably we want to try to minimize the number of new lines under most circumstances, but are we trying every permutation, or relying on some greedy matching algorithm that works most of the time?