Add :config-file cli option #165
Conversation
src/eca/config.clj
Outdated
| 1. base: fixed config var `eca.config/initial-config`. | ||
| 2. env var: searching for a `ECA_CONFIG` env var which should contains a valid json config. | ||
| 3. local config-file: searching from a local `.eca/config.json` file. | ||
| 4. `initializatonOptions` sent in `initialize` request." | ||
| 4. `initializatonOptions` sent in `initialize` request. | ||
| When a `ECA_CONFIGFILE` env var set, it takes precedence and we only deep merge: | ||
| 1. base: fixed config var `eca.config/initial-config`. | ||
| 2. env file: searching for a `ECA_CONFIGFILE` env var which points to a json config file." |
There was a problem hiding this comment.
I think we should just mention the env var in the existing waterfall, something like, maybe just add between 2 and 3?
There was a problem hiding this comment.
My original imp was to snug that into waterfall between 3/4. But then I realized that didn't make sense. When user supplied explicit config path to the config file, I don't think he'd (at least myself this case) expect that to be merged with local/home config in the waterfall.
There was a problem hiding this comment.
For example, I generated some eca config using home-manager. Then I go
ECA_CONFIGFILE=/nix/store/xxx/my.json eca server
That my.json is my exact pure config to start the server, I wouldn't expect it to be further merged with other stuff this case..
There was a problem hiding this comment.
I see your case, but:
- it's just easier to understand the waterfall with dumb logics like merging all posibilities, it's way easier to explain to users how that works
- I don't see why limit to not supporting passing 2 envs and merging them
It might be uncommon, but not impossible, if that makes code even easier to understand/maintain, I'd take that
There was a problem hiding this comment.
I can add 2 into the below case, so that when ECA_CONFIG and ECA_CONFIG_FILE both set, they get merged?
But the point is, when there is a option to set config explicitly, people usually expect it to be a replace logic instead of going down waterfall with local config.
For example, bb --config xxxx
--config <file> Replace bb.edn with file. Defaults to bb.edn adjacent to invoked file or bb.edn in current dir. Relative paths are resolved relative to bb.edn.
That means I want a explicit config to run with bb, not using my home config or what other config.
https://github.com/search?q=MANGOHUD_CONFIGFILE+NOT+is%3Afork+language%3ANix+&type=code
People set the config via that var to replace, not to go through the waterfall.
Or maybe we shall move ECA_CONFIG outside of the waterfall on top like this?
What do you think? This might be the best I think.
There was a problem hiding this comment.
Well, a slight mod. I think initializationOptions shall always go last on both cases, so it shall be something like this:
The top is the normal local case go through local config->home config chain, the bottom is the explicit set case:
There was a problem hiding this comment.
I'd still stick with a simple waterfall merge, config waterfalls not overwriting other configs is common, if we want to overwrite we should support in other way like cljfmt with extra-indents option not overriidng indents and so on.
There was a problem hiding this comment.
Check this comment I may have misunderstood the feature
CHANGELOG.md
Outdated
|
|
||
| ## Unreleased | ||
|
|
||
| - Add `ECA_CONFIGFILE` env var to allow point config file explicitly. |
There was a problem hiding this comment.
Can we call it ECA_CONFIG_FILE please?
|
@sg-qwt the feature looks good, just small changes requested |
sg-qwt
changed the title
|
@ericdallo pls check the new imp with cli option |
ericdallo
left a comment
ericdallo
left a comment
There was a problem hiding this comment.
Looks good! just a small thing about naming
|
Fixed the naming. |
|
|
||
| (def cli-spec | ||
| {:order [:help :version :verbose] | ||
| {:order [:help :version :verbose :config-file] |
There was a problem hiding this comment.
@ericdallo I noticed you left out --log-level in cli print, is that by design?
2 participants
Add :config-file cli option