-
Notifications
You must be signed in to change notification settings - Fork 10
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
Parse configuration file II: using serde, toml, merge & envy instead of config #111
Conversation
Thanks for the pull request Robin! I'll take a look this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for the pull request. I took a first look. I have a few comments, but overall looks very good to me! I'll sleep over it and give it another look later this week.
Thanks for the review! Maybe I should have made it clearer that this is just a draft with many rough edges. But I first wanted to reach consensus on the general approach before I polish the code. |
Oh, well, looked good to me (although I recognized it's a draft). The only other thing I can think of are are tests. E.g., to check the priorities of the different ways of setting configuration options. |
If we want to restrict the configuration to the top-level options, we could refactor these options into a This would allow us to remove the manual merge code. The drawback is that we would have to make bigger changes if we want to add configuration for subcommand options in the future. What do you think? |
Fixed some of the issues, rebased on devel. |
Thanks. Will take a look on the weekend or so. |
I think it looks great! |
Make it ready for review when you feel like it. |
Btw., feel free to merge commits adding a new crate as a dependency into the first one using it. I had separate commits for that in the past because of the subrepo magic we had (that checked in the respective source code). But without that I'd say it just adds noise and obfuscates why a dependency is introduced. But if you see value then keep it. |
|
Also, I have been thinking about dropping the
Users who prefer the macOS library or Windows AppData paths could still set the XDG environment variables. What do you think? |
I'd be fine with that, as long as it still works on those operating systems (which I can't test). So if we were to use |
I'd be fine with that, as long as it still works on those operating
systems (which I can't test). So if we were to use `xdg` on, say,
Windows (and assuming no configuration on the user's side), the
configuration would be expected at `%USERPROFILE%/.config/nitrocli`,
is that correct?
I was under the impression that this would be the case as the
documentation of the xdg crate does not mention any platform
restrictions and the lookup algorithm described in the
xdg::BaseDirectories documentation uses std::env::home_dir(), which
falls back to %USERPROFILE% on Windows.
But after looking at the implementation, it turns out that they set
#![cfg(any(unix, target_os = "redox"))]
in their lib.rs. So let’s stick with app-dirs2 or directories.
|
Rebased onto topic/anyhow. |
This patch implements basic configuration handling that reads a configuration file and stores the parsed data in the ExecCtx and RunCtx structs. It supports three configuration items: - model (previously only --model) - no_cache (previously only NITROCLI_NO_CACHE) - verbosity (previously only --verbose)
This patch uses the envy crate to parse the environment. A variable NITROCLI_KEY can be used to overwrite the configuration for *key*. This has the side effect that the NITROCLI_NO_CACHE variable is evaluated as a boolean variable (instead of only checking whether it is set). We also accept two new variables, NITROCLI_MODEL and NITROCLI_VERBOSITY.
This patch uses the directories crate to query the appropriate path for the configuration files. For Linux, paths according to the XDG Base Directory Specification are used. Note that directories does not yet support the XDG_CONFIG_DIRS variable for system-wide configuration files. Therefore we only use a user configuration file.
This patch adds a simple configuration file that demonstrates the syntax and contains some documentation. I suggest to ship this file together with nitrocli and to install it e. g. in the /usr/share/doc/nitrocli directory. This patch also adds a simple test case that makes sure that the example file is parsed correctly.
This patch adds a new --no-cache option that corresponds to the NITROCLI_NO_CACHE environment variable and the no_cache configuration. This makes the user interface more consistent as all configuration items are now backed by both an environment variable and a command-line option.
This patch updates the man page for the last changes: - new option --no-cache - changes to the environment variables - configuration files
I think I addressed all comments, so this should be ready for review and merge. |
Awesome! Thanks Robin. I'll have a final look later today and then we should be good to merge. |
All merged now with a few small changes. Thanks again! |
In pull request #101, I used the config crate to parse and merge the configuration from files and from the environment. config is apparently no longer maintained and uses an old version of the nom crate. This PR replaces config with serde and toml for the configuration file parsing, merge for the merging of different configuration sources, and envy for reading the configuration from the environment. Thus we no longer need the nom dependency and have more control over the dependency versions.
Note that we still have a separate
Config
struct that is manually merged with theArgs
struct. We could also use theArgs
struct directly, but the question is how to handle the subcommand enums. We could just skip them, or we could read the configuration for all subcommands into a map and then select the variant based on the CLI arguments.In the first version of the patch, I used the app_dirs crate to locate the config directories, but it was unmaintained and had some annoying bugs and limitations (andybarron/app-dirs-rs#34, andybarron/app-dirs-rs#12). We switched to the directories crate, but this crate does not support system configuration files. Now there is a maintained fork of app_dirs, app_dirs2, and I prepared patches for the two app_dirs issues (app-dirs-rs/app_dirs2#8, app-dirs-rs/app_dirs2#12). Once they are merged and released, I would move back to app_dirs2.