Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#336 extra-configs for importing extra-indents #337

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,5 @@ pom.xml.asc
.nrepl-port
reports
.cpcache
.clj-kondo
.lsp
Comment on lines +12 to +13
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes that aren't relevant to the PR should ideally be omitted.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are both standard things that clojure-lsp/IDE's might pull in, so figured they might be useful to add. I can yank it though if you really don't want them included

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are useful, but they're just not relevant to the current PR. A PR should only contain code to implement the feature/fix described.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I can yoink them then if them's the rules.

10 changes: 10 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,16 @@ In order to load the standard configuration file from Leiningen, add the
Paths can also be passed as command line arguments. If the path is
`-`, then the input is STDIN, and the output STDOUT.

* `:extra-configs` -
additional config files that will also be imported and merged into
the base configuration. Is not recursive, so `:extra-configs` in
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably should be recursive.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I could see that; I was worried about how this might be confusing or hard to debug. Some odd corner cases I was thinking about:

  • You pull in a config from a lib A, which references lib B, but you never pull in lib B's config, they end up missing or causing errors?
  • Could ordering get messy? thinking of multiple libraries using some other base library
  • Could the relative path references get messed up, depending on how they're pulled in?
  • Probably some others I'm missing?

If it's not recursive though, I could see inconveniences there as well. Not sure.

imported configs will not be respected.

File paths are expected to be relative paths from the base config file.

Only certain keys from other configs will be respected:
* `:extra-indents`

## License

Copyright © 2023 James Reeves
Expand Down
3 changes: 2 additions & 1 deletion cljfmt/project.clj
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,5 @@
:opts ["--static"
"--libc=musl"
"-H:CCompilerOption=-Wl,-z,stack-size=2097152"]}}
:provided {:dependencies [[org.clojure/clojurescript "1.11.60"]]}})
:provided {:dependencies [[org.clojure/clojurescript "1.11.60"]]}
:dev {:resource-paths ["test_resources"]}})
28 changes: 28 additions & 0 deletions cljfmt/src/cljfmt/config.clj
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,33 @@
(defn- directory? [path]
(some-> path io/file .getAbsoluteFile .isDirectory))

(def ^:private extra-config-respected-keys [:extra-indents])

(defn- merge-keys
[init coll ks]
(reduce (fn [acc k]
(update acc k merge (get coll k)))
init ks))

(defn- merge-extra-configs
[path config]
(let [base-dir (-> path io/file ((fn [^java.io.File f] (.getParent f))))]
;; extra-configs later in the list override earlier ones.
(loop [confs (mapv #(io/file base-dir %) (:extra-configs config))
econf {}]
(if (empty? confs)
;; A little confusing but: We're merging the base config's
;; values into the extra-configs, and then back into the base
;; config, so we allow the base config to have the final say.
(merge config (merge-keys econf config extra-config-respected-keys))
(let [;; Only merging select values from other configs.
xtra (-> confs first read-config
(select-keys extra-config-respected-keys))]
(recur (rest confs) ;;(merge econf xtra)
(reduce-kv (fn [acc k v]
(update acc k merge v))
econf xtra)))))))

(defn load-config
"Load a configuration merged with a map of sensible defaults. May take
an path to a config file, or to a directory to search. If no argument
Expand All @@ -70,4 +97,5 @@
path)]
(->> (some-> path read-config)
(merge default-config)
(merge-extra-configs path)
(convert-legacy-keys)))))
28 changes: 27 additions & 1 deletion cljfmt/test/cljfmt/config_test.clj
Original file line number Diff line number Diff line change
@@ -1,10 +1,36 @@
(ns cljfmt.config-test
(:require [cljfmt.config :as config]
[clojure.test :refer [deftest is]]))
[clojure.test :refer [deftest is testing]]
[clojure.java.io :as io]))

(deftest test-convert-legacy-keys
(is (= {:indents {'foo [[:inner 0]]}}
(config/convert-legacy-keys {:indents {'foo [[:inner 0]]}})))
(is (= {:extra-indents {'foo [[:inner 0]]}}
(config/convert-legacy-keys {:legacy/merge-indents? true
:indents {'foo [[:inner 0]]}}))))

(deftest test-load-config-extra-configs
(testing ":extra-configs allows you to import :extra-indents"
(is (= (config/load-config
(io/resource "cljfmt/config_test/empty-cljfmt.edn"))
(-> (config/load-config
(io/resource "cljfmt/config_test/cljfmt.edn"))
(dissoc :extra-configs)
(assoc :extra-indents {})))
(str "should only differ by :extra-indents (via :extra-configs)."
" All other keys are ignored."))
(is (= '{;; exists only in base config `test_resources/cljfmt.edn`
com.unrelated/a [[:inner 0]]
com.foo/a [[:inner 0]],
;; exists only in `test_resources/extra1-cljfmt.edn`
com.foo/b [[:inner 1]],
;; overwritten in `test_resources/extra2-cljfmt.edn`
com.foo/c [[:inner 2]],
com.foo/d [[:inner 2]],
;; exists only in `test_resources/extra2-cljfmt.edn`
com.foo/e [[:inner 2]]}
(:extra-indents (config/load-config
(io/resource "cljfmt/config_test/cljfmt.edn"))))
(str "should respect :extra-configs in order (later is higher-prio),"
" with base highest prio."))))
4 changes: 4 additions & 0 deletions cljfmt/test_resources/cljfmt/config_test/cljfmt.edn
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{:extra-indents {com.foo/a [[:inner 0]]
com.unrelated/a [[:inner 0]]}
:extra-configs ["extra1-cljfmt.edn"
"extra2-cljfmt.edn"]}
1 change: 1 addition & 0 deletions cljfmt/test_resources/cljfmt/config_test/empty-cljfmt.edn
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
11 changes: 11 additions & 0 deletions cljfmt/test_resources/cljfmt/config_test/extra1-cljfmt.edn
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{:parallel? true
:paths ["x"]
:sort-ns-references? true
:extra-indents {;; exists in base
com.foo/a [[:inner 1]]
;; not included in later config
com.foo/b [[:inner 1]]
;; overridden in later configs
com.foo/c [[:inner 1]]
com.foo/d [[:inner 1]]}
:extra-configs []}
7 changes: 7 additions & 0 deletions cljfmt/test_resources/cljfmt/config_test/extra2-cljfmt.edn
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{:parallel? false
:paths ["y"]
:sort-ns-references? false
:extra-indents {com.foo/c [[:inner 2]]
com.foo/d [[:inner 2]]
com.foo/e [[:inner 2]]}
:extra-configs []}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{:extra-configs ["config.edn"]}
Loading