Parse configuration file #101

robinkrahl · 2020-01-25T14:28:09Z

This patch series reads selected options (currently no_cache and model) from a
configuration file. This is only for discussion in #100. As I already
implemented it, I thought I might as well share it, even if we end up using a
different approach.

d-e-s-o · 2020-02-01T17:17:18Z

Just to make sure, there is nothing you are waiting on from me here, correct @robinkrahl ?

robinkrahl · 2020-02-01T18:36:43Z

I’m waiting on a decision on #100 – if you want to go with this approach, I can update the pull request with the missing documentation etc.

robinkrahl · 2020-02-02T23:31:37Z

I updated the pull request – I think it could be merged now.

I made these changes to the patch series:

Replace app_dirs with directories. app_dirs has not been updated for a while, and the author did not respond to my question regarding the maintenance status. By switching to directories, we lose access to system-wide configuration files, but we gain a more robust and better maintained dependency. (Problems with app_dirs include that it tries to create directories even if told not to, see get_app_dir creates directories despite the documentation saying otherwise andybarron/app-dirs-rs#34, and even if it supports system-wide configuration, it does not comply with the specification, see Consider doing something smarter with XDG multi-paths andybarron/app-dirs-rs#12).
Squash and re-order the commits to make the patch series more concise and to avoid unnecessary intermediate steps.
Add a --no-cache option for consistency.
Update the documentation and add an example file.
Add a simple unit test.

d-e-s-o · 2020-02-03T18:48:11Z

Thank you, Robin. I hope to take a closer look tomorrow. I'll probably merge it if it looks alright. From my perspective, it mostly comes down to technical details that differentiate the solutions and I don't have the time to experiment with alternative approaches right now.

d-e-s-o · 2020-02-05T01:52:15Z

Seems as if we are pulling in two version of arrayvec and now have a total of three mechanisms to deduce/do something based on the rustc version we work with.

robinkrahl · 2020-02-05T10:39:16Z

Sorry, I did not mean to close this, I was just cleaning up old branches.

Seems as if we are pulling in two version of arrayvec

Ultimately, nom is to blame here as they use an old version of lexical-core, see rust-bakery/nom/pull/1100 and rust-bakery/nom/pull/1101. (The pull request would not fix this as it updates lexical-core to 0.6, but the arrayvec dependency is updated in 0.7.)

now have a total of three mechanisms to deduce/do something based on the rustc version we work with.

As far as I see, they are similar but not the same:

rustc_version provides functions that are evaluated at runtime.
rustversion provides macros for conditional compilation.

Which one is the third?

d-e-s-o

Publishing this review as GitHub doesn't allow me to comment otherwise. Comments may be stale & irrelevant but I didn't want them to get lost.

d-e-s-o · 2020-02-04T17:59:15Z

nitrocli/src/args.rs

+
+    DeviceModel::from_str(v).map_err(|_| {
+      E::custom(format!(
+        "unexpected device model '{}', expected one of {}",


If I am not mistaken this string is contained in the final error in a way that only the expected type/values should be contained. Need to double check.

d-e-s-o · 2020-02-04T17:59:38Z

nitrocli/src/config.rs

  pub no_cache: bool,
+  #[serde(default)]


Nit: Move one line down.

d-e-s-o · 2020-02-04T18:03:40Z

nitrocli/CHANGELOG.md

@@ -11,6 +11,11 @@ Unreleased
 - Added support for configuration files:
  - Added `config` dependency in version `0.10.1`
  - Added `serde` dependency in version `1.0.104`
+- Reworked environment variables:
+  - Added the `NITROCLI_MODEL` and `NITROCLI_VERBOSITY` variables that set the


"variables" -> "environment variables"?

d-e-s-o · 2020-02-04T18:05:41Z

nitrocli/src/config.rs

@@ -37,9 +43,13 @@ pub struct Config {

 impl Config {
  pub fn load() -> Result<Self> {
+    let project_dirs = directories::ProjectDirs::from("", "", "nitrocli")
+      .ok_or_else(|| error::Error::from("Could not determine the home directory"))?;


Lower case the error message for consistency?

d-e-s-o · 2020-02-04T18:06:26Z

nitrocli/src/config.rs

@@ -37,9 +43,13 @@ pub struct Config {

 impl Config {
  pub fn load() -> Result<Self> {
+    let project_dirs = directories::ProjectDirs::from("", "", "nitrocli")


Mind introducing named variables for the two empty string arguments? It's not really clear what they refer to otherwise. With "nitrocli" one can at least take a guess.

d-e-s-o · 2020-02-04T18:07:46Z

nitrocli/src/tests/run.rs

+
+#[test]
+fn config_file() {
+    let mut config = ::config::Config::new();


Indentation too wide.

d-e-s-o · 2020-03-30T17:20:43Z

Seems as if we are pulling in two version of arrayvec

Ultimately, nom is to blame here as they use an old version of lexical-core, see Geal/nom/pull/1100 and Geal/nom/pull/1101. (The pull request would not fix this as it updates lexical-core to 0.6, but the arrayvec dependency is updated in 0.7.)

Thanks for digging. I understand, but irrespective of who's fault it is, it's still not nice and something I'd really want to avoid.

now have a total of three mechanisms to deduce/do something based on the rustc version we work with.

As far as I see, they are similar but not the same:

rustc_version provides functions that are evaluated at runtime.

rustversion provides macros for conditional compilation.

Which one is the third?

version_check is what I was referring to.

In the next patch, we will implement configuration handling using the config crate. This patch adds a ConfigError variant to the Error struct that wraps the config::ConfigError returned by the config crate.

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)

To make parsing easier, we use serde to deserialize the configuration data and store it in the Config struct. As the default Deserialize implementation for DeviceModel would only accept "Pro" and "Storage" and the Enum! macro does not allow per-field attributes, we manually implement Deserialize using the FromStr implementation.

This patch uses the config 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

d-e-s-o · 2020-08-05T01:02:08Z

Just noticing the update. Is this the updated pull request ready for review already, Robin?

robinkrahl · 2020-08-05T07:30:58Z

No, I just rebased it onto the current devel. I’ll open a new PR with the new implementation so that you can compare both, okay?

d-e-s-o · 2020-08-05T15:31:20Z

No, I just rebased it onto the current devel. I’ll open a new PR with the new implementation so that you can compare both, okay?

Sounds good.

d-e-s-o · 2020-09-02T05:42:09Z

We can probably close this pull request now that v2 has been merged.

d-e-s-o self-assigned this Jan 25, 2020

robinkrahl mentioned this pull request Jan 25, 2020

Configuration file #100

Closed

robinkrahl force-pushed the config branch from 7d4b330 to 78c1261 Compare February 2, 2020 23:25

robinkrahl changed the title ~~WIP: Parse configuration file~~ Parse configuration file Feb 2, 2020

robinkrahl force-pushed the config branch from 78c1261 to a1c90ef Compare February 2, 2020 23:36

robinkrahl closed this Feb 4, 2020

robinkrahl deleted the config branch February 4, 2020 20:07

robinkrahl restored the config branch February 5, 2020 10:24

robinkrahl reopened this Feb 5, 2020

d-e-s-o reviewed Mar 30, 2020

View reviewed changes

d-e-s-o force-pushed the devel branch from 5af33aa to c8a22ef Compare April 4, 2020 23:04

robinkrahl added 12 commits August 3, 2020 00:16

Update syn and proc-macro2 dependencies

f3a43d4

Add config dependency in version 0.10.1

d3b4c6c

Add ConfigError variant for config::ConfigError

0088914

In the next patch, we will implement configuration handling using the config crate. This patch adds a ConfigError variant to the Error struct that wraps the config::ConfigError returned by the config crate.

Add serde dependency in version 1.0.104

f123577

Add directories dependency in version 2.0.2

a75a160

Update man page

658bc29

This patch updates the man page for the last changes: - new option --no-cache - changes to the environment variables - configuration files

robinkrahl force-pushed the config branch from a1c90ef to 658bc29 Compare August 2, 2020 22:22

d-e-s-o marked this pull request as draft August 5, 2020 15:31

robinkrahl mentioned this pull request Aug 9, 2020

Parse configuration file II: using serde, toml, merge & envy instead of config #111

Closed

d-e-s-o force-pushed the devel branch from fd5edfe to 592d1a5 Compare September 1, 2020 04:24

d-e-s-o closed this Sep 2, 2020

robinkrahl deleted the config branch September 10, 2020 22:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse configuration file #101

Parse configuration file #101

robinkrahl commented Jan 25, 2020

d-e-s-o commented Feb 1, 2020

robinkrahl commented Feb 1, 2020 via email

robinkrahl commented Feb 2, 2020

d-e-s-o commented Feb 3, 2020

d-e-s-o commented Feb 5, 2020

robinkrahl commented Feb 5, 2020

d-e-s-o left a comment

d-e-s-o Feb 4, 2020

d-e-s-o Feb 4, 2020

d-e-s-o Feb 4, 2020

d-e-s-o Feb 4, 2020

d-e-s-o Feb 4, 2020

d-e-s-o Feb 4, 2020

d-e-s-o commented Mar 30, 2020

d-e-s-o commented Aug 5, 2020

robinkrahl commented Aug 5, 2020 via email

d-e-s-o commented Aug 5, 2020

d-e-s-o commented Sep 2, 2020

Parse configuration file #101

Parse configuration file #101

Conversation

robinkrahl commented Jan 25, 2020

d-e-s-o commented Feb 1, 2020

robinkrahl commented Feb 1, 2020 via email

robinkrahl commented Feb 2, 2020

d-e-s-o commented Feb 3, 2020

d-e-s-o commented Feb 5, 2020

robinkrahl commented Feb 5, 2020

d-e-s-o left a comment

Choose a reason for hiding this comment

d-e-s-o Feb 4, 2020

Choose a reason for hiding this comment

d-e-s-o Feb 4, 2020

Choose a reason for hiding this comment

d-e-s-o Feb 4, 2020

Choose a reason for hiding this comment

d-e-s-o Feb 4, 2020

Choose a reason for hiding this comment

d-e-s-o Feb 4, 2020

Choose a reason for hiding this comment

d-e-s-o Feb 4, 2020

Choose a reason for hiding this comment

d-e-s-o commented Mar 30, 2020

d-e-s-o commented Aug 5, 2020

robinkrahl commented Aug 5, 2020 via email

d-e-s-o commented Aug 5, 2020

d-e-s-o commented Sep 2, 2020