#336 extra-configs for importing extra-indents #337
Conversation
cljfmt/src/cljfmt/config.clj
Outdated
| (merge default-config) | ||
| (convert-legacy-keys))) | ||
|
|
||
| (defn- deep-merge |
There was a problem hiding this comment.
just stole quick from medley as an example. I'm not even sure if you'd want to deep merge, or instead only respect certain values in the config?
For example, I could see use-cases for both situations:
- I want to split my base config.edn up across multiple files, please deep-merge them all.
- I'm pulling in other librarie's config.edn's, all I want is their e.g. indentation configs, but I don't want any of their other configs overriding what I have.
There was a problem hiding this comment.
Merging configurations isn't a deep-merge normally in cljfmt, so it shouldn't be for :extra-configs. Instead, it should be a normal merge.
There was a problem hiding this comment.
Well, I don't really think it would make sense personally for each config to be wiping each others :extra-indents, as the whole point in my mind would be for them to be merged together. I assume you mean that it would make sense to just merge the :extra-indents into one another? I can do that pretty easily if that works?
There was a problem hiding this comment.
I've updated it to merge the :extra-indents maps together, and then merge the final :extra-indents back into the base config. If that's not what you meant or Ok let me know
There was a problem hiding this comment.
Hm, that's a fair point. Does deps.edn solve this in any particular way?
I don't like the idea of recursively merging configs, as that makes the process purely additive, and makes it different from how the default configuration is handled, and how options are handled.
I'm beginning to think that this issue might be better solved via a different means entirely. For example, if the use-case is just supporting extra indentations in libraries, we could create a indent resource in a standard location.
There was a problem hiding this comment.
Hm, that's a fair point. Does deps.edn solve this in any particular way?
I'm not familiar with deps.edn at all really, so unsure
I don't like the idea of recursively merging configs, as that makes the process purely additive, and makes it different from how the default configuration is handled, and how options are handled.
In my mind, this is new behavior is somewhat consistent with how :extra-indents works today, adding to the default :indents . I was also thinking that, since the base config has the final say, it could overwrite any values pulled in from downstream that it doesn't agree with, but that may be less than ideal, not sure.
I'm beginning to think that this issue might be better solved via a different means entirely. For example, if the use-case is just supporting extra indentations in libraries, we could create a indent resource in a standard location.
I'm not sure what you mean by this, but I'm open to other ideas/solutions
There was a problem hiding this comment.
I'm not sure what you mean by this, but I'm open to other ideas/solutions
clj-kondo has a mechanism for importing configurations from libraries. We could do something similar with cljfmt.
The way it would work is that cljfmt can optionally be given a classpath, which it will then search through to find indentation files in some predictable location. For example cljfmt.exports/*/*/indent.edn.
cljfmt/src/cljfmt/config.clj
Outdated
|
|
||
| (defn- load-extra-configs | ||
| "Load any :extra-configs specified in the base config file. | ||
| Note: is not recursive." |
There was a problem hiding this comment.
I could just see recursive configs becoming a mess very quickly. If doing something smarter, like e.g. looking at the classpath, then maybe something like that could work? idk.
juszczakn
force-pushed
the
issue/336-additional-configs
branch
from
20ce2ff
to
a9570e9
Compare
juszczakn
changed the title
| .clj-kondo | ||
| .lsp |
There was a problem hiding this comment.
Changes that aren't relevant to the PR should ideally be omitted.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok I can yoink them then if them's the rules.
cljfmt/src/cljfmt/config.clj
Outdated
| (merge default-config) | ||
| (convert-legacy-keys))) | ||
|
|
||
| (defn- deep-merge |
There was a problem hiding this comment.
Merging configurations isn't a deep-merge normally in cljfmt, so it shouldn't be for :extra-configs. Instead, it should be a normal merge.
| @@ -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 | |||
There was a problem hiding this comment.
It probably should be recursive.
There was a problem hiding this comment.
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.
juszczakn
force-pushed
the
issue/336-additional-configs
branch
from
a9570e9
to
dddf0a5
Compare
Allows pulling in :extra-indents from other configs. extra-configs are respected in order listed, so later configs overwrite earlier ones. :extra-indents in the base config override everything, allowing users to overrule indents pulled in that they don't agree with. :extra-configs file paths are expected to be relative to the base config.
juszczakn
force-pushed
the
issue/336-additional-configs
branch
from
dddf0a5
to
163cf02
Compare
#336 Initial shot at implementing the ability to import a subset of config values from additional configs found elsewhere.
My use-case is specifically the ability to import :extra-indents from other configs, and I could see other config values being a problem to import, so I limited the impl to just that.
Files are imported in the order listed, with later configs overriding earlier ones, and any values in the base cljfmt.edn overriding anything else, allowing projects to override imported indentation rules they don't agree with.
It made sense to me to respect relative paths for config files, since the use-case that I imagine is being able to import them from libraries, similar to clj-kondo with it's ability to export/import config values.