Skip to content

Refactor config updates#13802

Merged
sholderbach merged 32 commits intonushell:mainfrom
IanManske:config-update-value
Oct 11, 2024
Merged

Refactor config updates#13802
sholderbach merged 32 commits intonushell:mainfrom
IanManske:config-update-value

Conversation

@IanManske
Copy link
Copy Markdown
Member

@IanManske IanManske commented Sep 7, 2024

Description

This PR standardizes updates to the config through a new UpdateFromValue trait. For now, this trait is private in case we need to make changes to it.

Note that this PR adds some additional ShellError cases to create standard error messages for config errors. A follow-up PR will move usages of the old error cases to these new ones. This PR also uses Type::custom in lots of places (e.g., for string enums). Not sure if this is something we want to encourage.

User-Facing Changes

Should be none.

@cptpiepmatz
Copy link
Copy Markdown
Member

Wouldn't it be cool if UpdateFromValue would be a public trait? Plugin authors could use it for their own plugins that may take configs

@IanManske
Copy link
Copy Markdown
Member Author

Wouldn't it be cool if UpdateFromValue would be a public trait? Plugin authors could use it for their own plugins that may take configs

Yeah it would! In the future, we probably should. But right now it's a lot of boilerplate, since there's no macro 😉. Also, the interface may or may not need some edits, we'll see.

Comment on lines +110 to +122
/// A value's type did not match the expected type.
///
/// ## Resolution
///
/// Convert the value to the correct type or provide a value of the correct type.
#[error("Type mismatch")]
#[diagnostic(code(nu::shell::type_mismatch))]
RuntimeTypeMismatch {
expected: Type,
actual: Type,
#[label = "expected {expected}, but got {actual}"]
span: Span,
},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Mhh... #10698

Definitely better than the existing trash-heap of TypeMismatch. Not sure if we should give this name provisionally to something with just one span.

Comment on lines +79 to +85
let Value::Record { val: record, .. } = value else {
errors.type_mismatch(path, Type::record(), value);
return;
};

for (col, val) in record.iter() {
let path = &mut path.push(col);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This whole approach is certainly more modular but seeing this boilerplate all over the place also dilutes the important stuff.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yea... but the old code also had boilerplate everywhere and was actually unreadable 😄
In the future, a derive macro would be nice so that all of this code could be removed.

"vi_insert" => self.vi_insert.update(val, path, errors),
"vi_normal" => self.vi_normal.update(val, path, errors),
"emacs" => self.emacs.update(val, path, errors),
_ => errors.unknown_option(path, val),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will this new pattern still remove the invalid options from the backing Record or will they stick around and re-raise?

Copy link
Copy Markdown
Member Author

@IanManske IanManske Oct 1, 2024

Choose a reason for hiding this comment

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

The new config is turned back into a value which is then used to overwrite the old env value. So, invalid options are discarded in the process.

@IanManske IanManske marked this pull request as ready for review October 1, 2024 03:03
Comment on lines +214 to +220
if let Some(mut config) = self.get_env_var(engine_state, "config") {
let existing_config = self.get_config(engine_state);
let (new_config, error) = config.parse_as_config(&existing_config);
self.config = Some(new_config.into());
if let Some(value) = self.get_env_var(engine_state, "config") {
let old = self.get_config(engine_state);
let mut config = (*old).clone();
let error = config.update_from_value(&old, &value);
// The config value is modified by the update, so we should add it again
self.add_env_var("config".into(), config);
self.add_env_var("config".into(), config.clone().into_value(value.span()));
self.config = Some(config.into());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK, that is probably a performance regression.

Currently we clone the Value representation of the config once in Stack::get_env_var (somewhat unfortunate), then the existing Value::parse_as_config simply mutates the existing nested structure as needed for error correction. The final Value is then moved into the $env.config.

In the new version the Config struct is now also cloned and then all the nested records have to be allocated again by .into_value().

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If we changed get_env_var to return a reference to a value, then I think the performance would be similar (one value clone and one config allocation vs two config clones).

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Oct 2, 2024

We're getting close to the time where landing significant changes like this shouldn't be allowed because we need a good amount of time to test. Should we land this before the release? If so, please make the ci green.

@IanManske
Copy link
Copy Markdown
Member Author

Should be good to go, just had a merge conflict.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Oct 3, 2024

@sholderbach had some comments so let's let him get back to it before we land it to ensure we're all in sync.

Copy link
Copy Markdown
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

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

To stop the bitrot I am onboard with merging this after taking into account the conflict from #14020 .

While from a conceptual level I don't like the fact that we now separately crate a new config Value structure everytime, one benefit over the current implementation is that there will be no missing keys, which Doug was mentioning as a shortcoming a few meetings back.

We should monitor how this affects startup performance and if necessary we can refine things. I really like the fact that things move closer to their config types and its encouraged to use rich types. We have to watch out for folks that try to add new config stuff that they properly use the facilities you added. Mindless copy-paste coding still has a chance of producing bad outcomes.

@IanManske
Copy link
Copy Markdown
Member Author

While from a conceptual level I don't like the fact that we now separately crate a new config Value structure everytime

Not exactly sure what you mean by this. One nice thing about this PR is that there is now only one source of truth: the Config struct. The env value is then built off the struct, so they will always be in sync.

If it's a performance concern, I haven't yet benchmarked it, but it should be roughly the same. In the previous case we cloned the env value, and in this PR, we instead allocate a new one.

Anyways, yeah I agree we should monitor for regressions or other issues.

@sholderbach sholderbach merged commit fce6146 into nushell:main Oct 11, 2024
@hustcer hustcer added this to the v0.99.0 milestone Oct 12, 2024
@fdncred fdncred added the A:configuration Issue related to nu's configuration label Oct 12, 2024
IanManske added a commit that referenced this pull request Oct 13, 2024
# Description
Adds back the `to_ascii_lowercase` deleted in #13802. Also fixes the
error messages having the lowercased value instead of the original
value.
@qfel
Copy link
Copy Markdown
Contributor

qfel commented Oct 31, 2024

So IIUC the point of UpdateFromValue is to be able to merge configs (make updates from a record value). "Legacy" fields aside, could we leverage serde? Config already implements [De]Serialize, if we make Value a serialization format we could:

  • Convert Config to Value (serialize)
  • Merge with the fien Value (recursively, in a generic way)
  • Convert back to Config (deserialize)

I'm not sure how much code is needed to implement the Value-based [de]serialization, but at least the amount of boilerplate should be next to zero (due to automatic [De]Serialize implementations)

@IanManske
Copy link
Copy Markdown
Member Author

Unfortunately, the serde data model does not have some of the types/cases present in a nushell Value like datetime and cell path, and serde does not have an escape hatch like a (de)serialize_custom method. In order to do (de)serialization without discarding type information, some hacky workaround like the one done in the toml_datetime crate would be necessary.

But overall, I agree that using serde is appealing, especially since there are a bunch of other to {format} commands. What might work though, is to have some NuSerialize(r) and NuDeserialize(r) super traits. These would add methods for the types missing in the serde data model like (de)serialize_cellpath, and then also add (de)serialize_nu methods to perform (de)serialization with the augmented nushell data model. That could be something to explore....

@sholderbach
Copy link
Copy Markdown
Member

Further limitation for why we don't use serde for the Value <-> struct/enum mapping are the Spans in Value. The existing serde model is not really flexible to manage different choices there if I remember it correctly.

fdncred pushed a commit that referenced this pull request Nov 20, 2024
# Release-Notes Short Description

* Nushell now always loads its internal `default_env.nu` before the user
`env.nu` is loaded, then loads the internal `default_config.nu` before
the user's `config.nu` is loaded. This allows for a simpler
user-configuration experience. The Configuration Chapter of the Book
will be updated soon with the new behavior.

# Description

Implements the main ideas in #13671 and a few more:

* Users can now specify only the environment and config options they
want to override in *their* `env.nu` and `config.nu`and yet still have
access to all of the defaults:
* `default_env.nu` (internally defined) will be loaded whenever (and
before) the user's `env.nu` is loaded.
* `default_config.nu` (internally defined) will be loaded whenever (and
before) the user's `config.nu` is loaded.
* No more 900+ line config out-of-the-box.
* Faster startup (again): ~40-45% improvement in launch time with a
default configuration.
* New keys that are added to the defaults in the future will
automatically be available to all users after updating Nushell. No need
to regenerate config to get the new defaults.
* It is now possible to have different internal defaults (which will be
used with `-c` and scripts) vs. REPL defaults. This would have solved
many of the user complaints about the [`display_errors`
implementation](https://www.nushell.sh/blog/2024-09-17-nushell_0_98_0.html#non-zero-exit-codes-are-now-errors-toc).
* A basic "scaffold" `config.nu` and `env.nu` are created on first
launch (if the config directory isn't present).
* Improved "out-of-the-box" experience (OOBE) - No longer asks to create
the files; the minimal scaffolding will be automatically created. If
deleted, they will not be regenerated. This provides a better
"out-of-the-box" experience for the user as they no longer have to make
this decision (without much info on the pros or cons) when first
launching.
* <s>(New: 2024-11-07) Runs the env_conversions process after the
`default_env.nu` is loaded so that users can treat `Path`/`PATH` as
lists in their own config.</s>
* (New: 2024-11-08) Given the changes in #13802, `default_config.nu`
will be a minimal file to minimize load-times. This shaves another (on
my system) ~3ms off the base launch time.
* Related: Keybindings, menus, and hooks that are already internal
defaults are no longer duplicated in `$env.config`. The documentation
will be updated to cover these scenarios.
* (New: 2024-11-08) Move existing "full" `default_config.nu` to
`sample_config.nu` for short-term "documentation" purposes.
* (New: 2024-11-18) Move the `dark-theme` and `light-theme` to Standard
Library and demonstrate their use - Also improves startup times, but
we're reaching the limit of optimization.
* (New: 2024-11-18) Extensively documented/commented `sample_env.nu` and
`sample_config.nu`. These can be displayed in-shell using (for example)
`config nu --sample | nu-highlight | less -R`. Note: Much of this will
eventually be moved to or (some) duplicated in the Doc. But for now,
this some nice in-shell doc that replaces the older
"commented/documented default".
* (New: 2024-11-20) Runs the `ENV_CONVERSIONS` process (1) after the
`default_env.nu` (allows `PATH` to be used as a list in user's `env.nu`)
and (2) before `default_config.nu` is loaded (allows user's
`ENV_CONVERSIONS` from their `env.nu` to be used in their `config.nu`).
* <s>(New: 2024-11-20) The default `ENV_CONVERSIONS` is now an empty
record. The internal Rust code handles `PATH` (and variants) conversions
regardless of the `ENV_CONVERSIONS` variable. This shaves a *very* small
amount of time off the startup.</s> Reset - Looks like there might be a
bug in `nu-enginer::env::ensure_path()` on Windows that would need to be
fixed in order for this to work.

# User-Facing Changes

By default, you shouldn't see much, if any, change when running this
with your existing configuration.

To see the greatest benefit from these changes, you'll probably want to
start with a "fresh" config. This can be easily tested using something
like:

```nushell
let temp_home = (mktemp -d)
$env.XDG_CONFIG_HOME = $temp_home
$env.XDG_DATA_HOME = $temp_home
./target/release/nu
```

You should see a message where the (mostly empty) `env.nu` and
`config.nu` are created on first start. Defaults should be the same (or
similar to) those before the PR. Please let me know if you notice any
differences.

---

Users should now specify configuration in terms of overrides of each
setting. For instance, rather than modifying `history` settings in the
monolithic `config.nu`, the following is recommended in an updated
`config.nu`:

```nu
$env.config.history = {
  file_format: sqlite,
  sync_on_enter: true
  isolation: true
  max_size: 1_000_000
}
```

or even just:

```nu
$env.config.history.file_format = sqlite
$env.config.history.isolation: true
$env.config.history.max_size = 1_000_000
```

Note: It seems many users are already appending a `source my_config.nu`
(or similar pattern) to the end of the existing `config.nu` to make
updates easier. In this case, they will likely want to remove all of the
previous defaults and just move their `my_config.nu` to `config.nu`.

Note: It should be unlikely that there are any breaking changes here,
but there's a slim chance that some code, somewhere, *expects* an
absence of certain config values. Otherwise, all config values are
available before and after this change.

# Tests + Formatting

- :green_circle: `toolkit fmt`
- :green_circle: `toolkit clippy`
- :green_circle: `toolkit test`
- :green_circle: `toolkit test stdlib`

# After Submitting

Configuration Chapter (and related) of the doc is currently WIP and will
be finished in time for 0.101 release.
WindSoilder pushed a commit that referenced this pull request Nov 27, 2024
# Description

As a bit of a follow-on to #13802 and #14249, this (pretty much a
"one-line" change) really does *always* populate the `$env.config`
record with the `nu-protocol::config` defaults during startup. This
means that an `$env.config` record is value (with defaults) even during:

* `nu -n` to suppress loading of config files
* `nu -c <commandstring>`
* `nu <script>`

# User-Facing Changes

There should be no case in which there isn't a valid `$env.config`.

* Before:

  ```nushell
  nu -c "$env.config"
  # -> Error
  ```

* After:

  ```nushell
  nu -c "$env.config"
  # -> Default $env.config record
  ```

Startup time impact is negligible (17.072µs from `perf!` on my system) -
Seems well worth it.

# Tests + Formatting

Added tests for several `-n -c` cases.

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

Config chapter update still in progress.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A:configuration Issue related to nu's configuration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants