weavejester/cljfmt

`reformat-string` doesn't honor `:legacy/merge-indents? true`

PEZ opened this issue · 8 comments

PEZ commented

Hello! With version 0.11.2 I don't get :legacy/merge-indents? true to work. The :indents seem to replace all defaults:

 (cljfmt/reformat-string "(defn f\n[])" {:legacy/merge-indents? true
                                         :indents '{foo [[:block 0]]}})
 ;; => "(defn f\n    [])"

Is this the expected behaviour?

I'm running in to this with Calva, which uses reformat-string for its formatting, and I'd like to be able to instruct users to use the legacy merge if they for some reason can't use :extra-indents yet.

weavejester commented

Yes, it's expected. The :indents key on reformat-string has always replaced the existing indentation; the merging behavior was part of logic for loading the configuration (whether via Leiningen or otherwise), which has now been consolidated in the cljfmt.config namespace.

If you use cljfmt.config/load-config to load the configuration file, it should correctly interpret the legacy key.

PEZ commented

Thanks! We are using cljfmt from ClojureScript so I don't think cljfmt.config is available to us. We used to merge the configs on our end, but that stopped working because we relied on a cljfmt-internal function to sort the rules and provide the sorted results to reformat-string.

I haven't seen anyone requesting to use the legacy merge yet. (We updated to latest cljfmt just a few days ago.) I've updated the Calva documentation with a note about this.

weavejester commented

Ah, I see. We could create a node.js version of cljfmt.config in order to harmonize the configuration reading; not just for legacy keys, but also for finding and reading the configuration.

PEZ commented

I like that idea. Currently Calva has a configuration pointing it at the config file, and as long as we can keep that working I think it would be great if things could be expected to work similarly as with the CLI.

To give a fuller picture of Calva's cljfmt usage I should add that we also have an indenter, written in TypeScript, that reads its indent configuration from the same file, and also from cljfmt/default-indents. (I have yet to update it to correctly handle the 0.11.x configuration.) As long as we still have cljfmt/default-indents we should be good. But maybe we would also need a way to get at the :extra-indents, if we start to rely on cljfmt.config for finding and read the config file.

PEZ commented

Let me check a thing. I am considering a workaround for now, where Calva would move the :indents to :extra-indents in the presence of the :legacy/merge-indents? true, and hand that to reformat-string. Would that work, or have I misunderstood the semantics?

weavejester commented

Yes, that's exactly what cljfmt does:

(defn convert-legacy-keys [config]
  (cond-> config
    (:legacy/merge-indents? config)
    (-> (assoc :extra-indents (:indents config))
        (dissoc :legacy/merge-indents?
                :indents))))
  • ❤️1
PEZ commented

Awesome. I copied that function. 😄

I just released a new Calva where users can now use the legacy merge key in the config, so I'm good with this issue being closed.

I'm still interested in being able to use cljfmt for the config file logic, but maybe that's a separate issue and discussion?

weavejester commented

Probably best to open a separate issue for ClojureScript support of the cljfmt.config namespace.

  • 👍1