Refactor and fix Config<->Value mechanism#10896
Conversation
There is a lot of work waiting as the record recovery mechanism requires us to modify. (e.g. see the `invalid_key!` macro and the reverse iteration for modification)
Instead of having to deal with two separate vecs, indices and reverse iteration for safe removal, let's use `Record::retain_mut`. Things can be further simplified as we now use `&mut Value` for most of the things directly. The remaining helper macros are simplified to generate their relevant `Span`s from the `Value`. As each `retain_mut` that yields values binds a `span` variable, the spans should now be more likely correct. Open work: - Fix the help messages to use correct paths (see the whole `key2` stuff) - Some things do not properly restore - padding does not block setting the value
Switch over to using the `key` arguments of the closure directly for better readability.
`Record` now provides a safe `.get()`
Let's split up this bad boy
`None` is less readable. Also should simpify things if we want to define `T.parse`/`T.recover` pairs
Demo the use of `FromStr` and a direct recovery method
Move the error handling to this helper. Leverage the `ReconstructVal` and `FromStr` traits
This is not perfect if the string is already illegal. See nushell#9500
Getting rid of the helper macros will allow us to split up the config processing into smaller parse rules that all follow the pattern: ``` fn process_stuff(&mut config_point, &mut Value, &mut errors) ```
This entails a minimal breaking change as we will not fall back to `FooterMode::Never` when failing to parse.
- Make "rounded" global default - Elide a forced clone
|
Thanks for taking this on. It's been needed for a while. I'm excited to see your PRs. |
|
One problem with config is also that there is one global Config struct, but there would need to be one config per overlay to work correctly, see also #7760. I'm thinking, do we need the Config struct at all, or could we just use the |
|
Always using a Value from the environment would also solve the problem encountered in #10483: No need to clone the entire Config just to get one value from it. With the new ergonomic Record API, I'm becoming increasingly in favor of ditching the Config struct as it would allow us to sidestep many bugs, e.g., #7760 #10175. |
Yes and no. As long as we don't have a mechanism to store and materialize diffs of nested values or another elegant copy-on-write mechanism that allows us to cheaply keep track of the different In general duplicating the information sucks. At the same time having explicit types is much better than matching on In my view we should go ahead with this PR and then in a follow up we can break up the big processing method into smaller processing functions that take care of the subrecords for better readability. Then we can do a survey of which config options are read in a hot-loop or which are cold enough that the |
|
I'd vote to land this sooner rather than later. We're getting close to release date. |
|
Agreed, I'd land it, I just wanted to share some thoughts I had on this topic. |
# Description Our config exists both as a `Config` struct for internal consumption and as a `Value`. The latter is exposed through `$env.config` and can be both set and read. Thus we have a complex bug-prone mechanism, that reads a `Value` and then tries to plug anything where the value is unrepresentable in `Config` with the correct state from `Config`. The parsing involves therefore mutation of the `Value` in a nested `Record` structure. Previously this was wholy done manually, with indices. To enable deletion for example, things had to be iterated over from the back. Also things were indexed in a bunch of places. This was hard to read and an invitation for bugs. With nushell#10876 we can now use `Record::retain_mut` to traverse the records, modify anything that needs fixing, and drop invalid fields. # Parts: - Error messages now consistently use the correct spans pointing to the problematic value and the paths displayed in some messages are also aligned with the keys used for lookup. - Reconstruction of values has been fixed for: - `table.padding` - `buffer_editor` - `hooks.command_not_found` - `datetime_format` (partial solution) - Fix validation of `table.padding` input so value is not set (and underflows `usize` causing `table` to run forever with negative values) - New proper types for settings. Fully validated enums instead of strings: - `config.edit_mode` -> `EditMode` - Don't fall back to vi-mode on invalid string - `config.table.mode` -> `TableMode` - there is still a fall back to `rounded` if given an invalid `TableMode` as argument to the `nu` binary - `config.completions.algorithm` -> `CompletionAlgorithm` - `config.error_style` -> `ErrorStyle` - don't implicitly fall back to `fancy` when given an invalid value. - This should also shrink the size of `Config` as instead of 4x24 bytes those fields now need only 4x1 bytes in `Config` - Completely removed macros relying on the scope of `Value::into_config` so we can break it up into smaller parts in the future. - Factored everything into smaller files with the types and helpers for particular topics. - `NuCursorShape` now explicitly expresses the `Inherit` setting. conversion to option only happens at the interface to `reedline`
# Description Our config exists both as a `Config` struct for internal consumption and as a `Value`. The latter is exposed through `$env.config` and can be both set and read. Thus we have a complex bug-prone mechanism, that reads a `Value` and then tries to plug anything where the value is unrepresentable in `Config` with the correct state from `Config`. The parsing involves therefore mutation of the `Value` in a nested `Record` structure. Previously this was wholy done manually, with indices. To enable deletion for example, things had to be iterated over from the back. Also things were indexed in a bunch of places. This was hard to read and an invitation for bugs. With nushell#10876 we can now use `Record::retain_mut` to traverse the records, modify anything that needs fixing, and drop invalid fields. # Parts: - Error messages now consistently use the correct spans pointing to the problematic value and the paths displayed in some messages are also aligned with the keys used for lookup. - Reconstruction of values has been fixed for: - `table.padding` - `buffer_editor` - `hooks.command_not_found` - `datetime_format` (partial solution) - Fix validation of `table.padding` input so value is not set (and underflows `usize` causing `table` to run forever with negative values) - New proper types for settings. Fully validated enums instead of strings: - `config.edit_mode` -> `EditMode` - Don't fall back to vi-mode on invalid string - `config.table.mode` -> `TableMode` - there is still a fall back to `rounded` if given an invalid `TableMode` as argument to the `nu` binary - `config.completions.algorithm` -> `CompletionAlgorithm` - `config.error_style` -> `ErrorStyle` - don't implicitly fall back to `fancy` when given an invalid value. - This should also shrink the size of `Config` as instead of 4x24 bytes those fields now need only 4x1 bytes in `Config` - Completely removed macros relying on the scope of `Value::into_config` so we can break it up into smaller parts in the future. - Factored everything into smaller files with the types and helpers for particular topics. - `NuCursorShape` now explicitly expresses the `Inherit` setting. conversion to option only happens at the interface to `reedline`
Description
Our config exists both as a
Configstruct for internal consumption and as aValue. The latter is exposed through$env.configand can be both set and read.Thus we have a complex bug-prone mechanism, that reads a
Valueand then tries to plug anything where the value is unrepresentable inConfigwith the correct state fromConfig.The parsing involves therefore mutation of the
Valuein a nestedRecordstructure. Previously this was wholy done manually, with indices.To enable deletion for example, things had to be iterated over from the back. Also things were indexed in a bunch of places. This was hard to read and an invitation for bugs.
With #10876 we can now use
Record::retain_mutto traverse the records, modify anything that needs fixing, and drop invalid fields.Parts:
table.paddingbuffer_editorhooks.command_not_founddatetime_format(partial solution)table.paddinginput so value is not set (and underflowsusizecausingtableto run forever with negative values)config.edit_mode->EditModeconfig.table.mode->TableModeroundedif given an invalidTableModeas argument to thenubinaryconfig.completions.algorithm->CompletionAlgorithmconfig.error_style->ErrorStylefancywhen given an invalid value.Configas instead of 4x24 bytes those fields now need only 4x1 bytes inConfigValue::into_configso we can break it up into smaller parts in the future.NuCursorShapenow explicitly expresses theInheritsetting. conversion to option only happens at the interface toreedline