weavejester/cljfmt

Incorrect cljfmt.config/load-config defaults merge

ai212983 opened this issue ยท 5 comments

Hello guys,

Today, I encountered an issue with cljfmt CLI tool. It seems to be ignoring the {:indents ^:replace {#".*" [[:inner 0]]}} configuration line. After some tinkering, I managed to create a reproducible example using bb.

The configuration file cljfmt.clj looks like this:

{:indents ^:replace {#".*" [[:inner 0]]
let [[:inner 0]]}

:test-code
(let
[]
(when
(= :indents :replaced)
:hello?))}

Additionally, there's the bb-check.clj file:

#!/usr/bin/env bb

(require '[cljfmt.core :as core]
         '[cljfmt.config :as cfg])

(let [{:keys [indents] :as config} (cfg/load-config)
      config-clj (slurp "./cljfmt.clj")]
  (println "Reformatted cljfmt.clj with manually loaded config:\n\n" (core/reformat-string config-clj (read-string config-clj)) "\n\n")
  (println "Reformatted cljfmt.clj with cljfmt loaded config:\n\n" (core/reformat-string config-clj config) "\n\n")
  (println "Indents configuration:")
  (println "   for `let`: " (get indents 'let))
  (println "   for `when`: " (get indents 'when)))

And also bb.edn:

{:deps {dev.weavejester/cljfmt {:mvn/version "0.10.6"}}}

The output would be as follows:

Reformatted cljfmt.clj with manually loaded config:

 {:indents ^:replace {#".*" [[:inner 0]]
                     let [[:inner 0]]}

 :test-code
 (let
   []
   (when
     (= :indents :replaced)
     :hello?))}


Reformatted cljfmt.clj with cljfmt loaded config:

 {:indents ^:replace {#".*" [[:inner 0]]
                     let [[:inner 0]]}

 :test-code
 (let
   []
   (when
    (= :indents :replaced)
     :hello?))}


Indents configuration:
   for `let`:  [[:inner 0]]
   for `when`:  [[:block 1]]

let is overridden in :indents directly and indented as expected in both cases. However, when behaves correctly only with the manually loaded config and not with cljfmt.config/load-config.

I assume that load-config may not be merging with defaults properly, but I might be missing something here. If so, please point me to the relevant ticket or documentation, as I couldn't find any :(.

Thank you.

edit: added bb.edn file

The ^:replace metadata is specific to Leiningen. It looks like the INDENTS.md document hasn't been updated to account for that.

We could add in metadata merging, though I'm leaning toward adding this feature via an :extra-indents option instead. i.e. :indents would replace, while :extra-indents would extend, similar to how :deps and :extra-deps work in a deps.edn file.

@weavejester Oh, thanks for the prompt reply!

Does :intents and :extra-indents allows batch rules replacements though? What I am looking for is

For example, to replace all indentation rules with a constant 2-space indentation:

{:indents ^:replace {#".*" [[:inner 0]]}}

Yes. So:

Leiningen cljfmt.edn
{:indents ^:replace {...}} {:indents {...}}
{:indents {...}} {:extra-indents {...}}

Obviously the downside to this is that it breaks backward compatibility in this particular place. Other options are:

  • to use meta-merge
  • to have a :replace-indents or :base-indents key

However, the advantage is that it follows the existing conventions of deps.edn.

@weavejester I hope it will be implemented soon enough, because currently I have to either override everything in cljfmt.clj or use bb script to manually load config.

I've uploaded a fix to master, so you can compile it manually, but I'm unsure if I'll be able to release 0.11.0 this week or next week as some more changes will be necessary.