weavejester / cljfmt

A tool for formatting Clojure code
Eclipse Public License 1.0
1.11k stars 120 forks source link

Incorrect cljfmt.config/load-config defaults merge #312

Closed ai212983 closed 1 year ago

ai212983 commented 1 year ago

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

weavejester commented 1 year ago

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.

ai212983 commented 1 year ago

@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]]}}
weavejester commented 1 year ago

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:

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

ai212983 commented 1 year ago

@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.

weavejester commented 1 year ago

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.