Grouped config commands better (closes #6911)

[webbedspace]

Description

• Closes config: command-specific options could be grouped better #6911. The new structure of the default config is this:

  ls: {
    use_ls_colors: true # use the LS_COLORS environment variable to colorize output
    clickable_links: true # enable or disable clickable links. Your terminal has to support links.
  }
  rm: {
    always_trash: true # always act as if -t was given. Can be overridden with -p
  }
  cd: {
    abbreviations: true # allows `cd s/o/f` to expand to `cd some/other/folder`
  }
  table: {
    mode: rounded # basic, compact, compact_double, light, thin, with_love, rounded, reinforced, heavy, none, other
    index_mode: always # "always" show indexes, "never" show indexes, "auto" = show indexes when a table has "index" column
    trim: {
      methodology: wrapping # wrapping or truncating
      wrapping_try_keep_words: true # A strategy used by the 'wrapping' methodology
      truncating_suffix: "..." # A suffix used by the 'truncating' methodology
    }
  }
  history: {
    max_size: 10000 # Session has to be reloaded for this to take effect
    sync_on_enter: true # Enable to share history between multiple sessions, else you have to close the session to write history to file
    file_format: "plaintext" # "sqlite" or "plaintext"
  }
  completions: {
    case_sensitive: false # set to true to enable case-sensitive completions
    quick: true  # set this to false to prevent auto-selecting completions when only one remains
    partial: true  # set this to false to prevent partial filling of the prompt
    algorithm: "prefix"  # prefix or fuzzy
    external: {
      enable: true # set to false to prevent nushell looking into $env.PATH to find more suggestions, `false` recommended for WSL users as this look up my be very slow
      max_results: 100 # setting it lower can improve completion performance at the cost of omitting some options
      completer: null # check 'carapace_completer' above as an example
    }
  }
  filesize: {
    metric: true # true => KB, MB, GB (ISO standard), false => KiB, MiB, GiB (Windows standard)
    format: "auto" # b, kb, kib, mb, mib, gb, gib, tb, tib, pb, pib, eb, eib, zb, zib, auto
  }

All old options' positions continue to be read as usual, and there are currently no deprecation warnings given.

• Changes the default always_trash option to true because I feel it's a safer default for basic usage. (negotiable)

• Also tweaks the wording of rm's help in a few places.

Tests + Formatting

Make sure you've done the following, if applicable:

• Add tests that cover your changes (either in the command examples, the crate/tests folder, or in the /tests folder)
  • Try to think about corner cases and various ways how your changes could break. Cover those in the tests

Make sure you've run and fixed any issues with these commands:

• cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
• cargo clippy --workspace --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
• cargo test --workspace --features=extra to check that all tests pass

User-Facing Changes

If you're making changes that will affect the user experience of Nushell (ex: adding/removing a command, changing an input/output type, adding a new flag):

• Get another regular contributor to review the PR before merging
• Make sure that there is an entry in the documentation (https://github.com/nushell/nushell.github.io) for the feature, and update it if necessary

[fdncred]

I think this is a good organization step to take but it worries me since it will literally break everyone's configuration. I'm not sure I'm prepared to answer 1000 questions of "what's wrong with my config". :)

[rgwood]

Agreed with @fdncred; this seems like a good change but breaking users is a concern. Some ideas off the top of my head:

• Wait until a big release (like v0.80 or v1.0?) with other breaking changes, get it all over with at once
• Invest in some kind of config-migration functionality

[webbedspace]

I think this is a good organization step to take but it worries me since it will literally break everyone's configuration.

I just said:

All old options' positions continue to be read as usual

If you'll check the code, you'll see I didn't delete any of the old branches from config.rs Config::into_config()

[webbedspace]

Also, can someone tell me why only the macos CI test suddenly doesn't work, and why it seems that the CI framework is injecting .DS_Store files into the test runner's directories:

---- commands::rm::allows_doubly_specified_file stdout ----
=== stderr

thread 'commands::rm::allows_doubly_specified_file' panicked at 'assertion failed: `(left == right)`
  left: `["/private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/.tmpmMcaiY/rm_test_12/.DS_Store"]`,
 right: `[]`', crates/nu-command/tests/commands/rm.rs:258:9
stack backtrace:

[fdncred]

There's a way to turn off .DS_Store files on MacOS, but I don't remember what it is. We should probably figure that out and put it in our CI I guess. It's weird though that it just started popping up. We've been building MacOS for years without issue.

[rgwood]

All old options' positions continue to be read as usual

Ah, gotcha. I see it now, nicely done.

After a quick skim your approach looks good to me, but I'll need to sit down and take a closer look at the extensive config.rs changes.

[fdncred]

I guess we have to work around it. This doesn't sound encouraging.

[fdncred]

just for fun, I restarted the macOS test because the .ds_store problem doesn't show up anywhere else. I'm wondering if it was just some type of ci glitch.

[rgwood]

Looking at this again...

I think it would make sense to split this into 2 PRs. The changes to rm (documentation tweaks, a new test) seem unrelated to the larger config change.

[webbedspace]

I think it would make sense to split this into 2 PRs. The changes to rm (documentation tweaks, a new test) seem unrelated to the larger config change.

I had to change rm's help docs because they referenced rm_always_trash. I did the other tweaks because I was "in the neighbourhood" and they seemed too minor for their own PR. Although, I might as well drop the extra test case.

[rgwood]

I'm also not a fan of changing the always_trash default in this PR. I can see how that change might make sense, but it is a pretty large behaviour change. Should probably get more discussion first and I don't think it should be bundled with a change to the config format/layout.

[webbedspace]

Well, alright.

[rgwood]

Thanks. I think this is looking good but let's wait until after next release (Nov 8) to merge it; that will give us 3 weeks to test it internally before we unleash it on users.

[fdncred]

Thinking about this again, this is going to make on-the-fly updates a bit different too. For instance, now to change the table_mode, we can do something like let config = ($env.config | upsert table_mode light). With this PR we'd have to change that to let config = ($env.config | upsert table.mode light). i.e. we'll have to use nested cell paths for updating. I haven't actually tried that yet, but I think my syntax is correct. Some of these go 3 levels deep like table.trim.methodology - hopefully all that still works after this PR.

[webbedspace]

@fdncred This is wrong; this PR only changes how the config file is read; the actual structure of $env.config remains the same for exactly these compatibility reasons. Eventually they should change to actually resemble the config file's Nuon structure, though.

[fdncred]

@webbedspace Correct me if I'm wrong, but you're saying this because all the entries are in this code twice now; like they were originally and how they are in your new grouped methodology. Is that right?

So, I see things like use_ls_colors in there twice. I believe you did this stop from having a breaking change. I think that's what you mention above.

So, at some point, when there is a breaking change, and only grouped config is available, my point above will be valid then. I realize I'm restating a lot but I'm trying to make sure I'm understanding everything.

I'm really not sure how I feel about this being either a massive breaking change (bad for users) or duplicate values in the code (bad for developers). I think the core-team just needs to decide whether we're going to accept this or not.

[webbedspace]

Your comment made me realise I'd had a slight misunderstanding. My apologies. I'd assumed that EngineState (in engine_state.rs) generated the $env.config environment variable whenever it was read, so that it always A) correctly mirrored its actual key-value state, and B) filtered out illegal config values. However, I now see that it does nothing of the sort, and it just saves the record on the right side of let-env config = as-is. (Personally, I think it should do both of the above, although there might be good reasons why it can't.)

Fortunately for me, this doesn't really change my point very much. The reality is the following:

• If your config.nu's let-env config = uses the old positions (such as cd_with_abbreviations) then $env.config will still have $env.config.cd_with_abbreviations`. (← essentially what I'd originally meant)
• If your config.nu's let-env config = uses the new positions (such as cd: { abbreviations }) then $env.config will have $env.config.cd.abbreviations`. (Good because people using the new positions would expect $env.config to have this structure)

This does mean something funny I hadn't noticed until now: mixing the old and new values (i.e. having let-env config = ($env.config | merge { cd: { abbreviations: false }) with let-env config = ($env.config | merge { cd_with_abbreviations: false }) means that the resulting $env.config will have both positions on it, AND one of them could be out-of-date with the actual system state (i.e. EngineState.config).

I understand this sounds unseemly, but A) I don't believe mixing the two is even all that likely to occur in normal usage (people will either use one or the other), B) reading $env.config to query the system state is already very flimsy and brittle (one can just blow away $env.config altogether with let-env config = 'foobarbaz' and it will "work") and C) I am absolutely willing to change this if you'd like (such as, make an accompanying pullreq where reading $env.config constructs a new record from EngineState.config, this avoiding most of these issues by having it be the "source of truth" at all times. Or, alternatively, have into_config() edit a clone of the passed-in record, and put that on the env_vars instead.)

I'm really not sure how I feel about this being either a massive breaking change (bad for users) or duplicate values in the code (bad for developers).

I tried to make it not actually that bad for developers: all of the "legacy" config position reads in into_config() have been carefully sorted between comments reading // Legacy config options (deprecated as of 2022-11-02) and // End legacy options.

[webbedspace]

Thanks. I think this is looking good but let's wait until after next release (Nov 8) to merge it; that will give us 3 weeks to test it internally before we unleash it on users.

So, um, it's been a week and a half… is this still OK to merge?

[fdncred]

@webbedspace Please remove the rm changes and put them in another PR. They are simple to land. The config changes are still up for debate.

I'm personally still worried about unintended consequences. I know some people use the default_config.nu files and then source changes on top of them. I'm wondering how this behavior will work with this. Meaning, the default_config.nu is in the "new" format and their source blah blah changes are in the "old" format. I'm thinking this will break for them. I think @kubouch does something like this.

Generally, I agree that people mostly will use one format or the other though.

I'm also considering that maybe we want to have a deprecation message somewhere and say something like, "We have changed the format of the config.nu file. Please see nushell.sh/book/some/link. The "old" format will be deprecated in 0.80 and only the new format will work at that time.". Maybe not that verbose but you get the idea. I just don't like having two flavors for very long.

[webbedspace]

I already reverted the always_trash default change in 922c6a3. If you mean I must also revert the help line changes, well… okay, but I think they're all rather prima-facie beneficial.

[fdncred]

@webbedspace what I'm talking about is removing all changes to rm.rs out of this PR. This is what I see when I look at the files.

The reason I requested this is because (1) all those changes to rm.rs looked good and we can easily land in another PR (2) if we land them in this PR and then end up rolling this PR back due to unforeseen config problems, then all the rm.rs changes get rolled back too.

[webbedspace]

Well, y'see, I need to change the line "Delete or move a file to the system trash (depending on 'rm_always_trash' config option)" because rm_always_trash has changed to always_trash (more specifically rm.always_trash), and if I leave it, then it will refer to it incorrectly. I also need to change Cannot execute `rm`; the current configuration specifies `rm_always_trash = true`, but the current nu executable was not built with feature `trash_support` or trash is not supported on your platform. for the same reason.

[fdncred]

I'm sorry, you're right. Those changes are related I was missing those points.

[webbedspace]

I added a deprecation warning message. It reads:

The format of $env.config has recently changed, and several options have been grouped into sub-records. You may need to update your config.nu file. Please consult https://www.nushell.sh/blog/2022-11-29-nushell-0.72.html for details. Support for the old format will be removed in version 0.80.

The URL is made up, for now.

[kubouch]

I quite like the new grouped config options. For me, it would mean I would need to reorganize my config a bit but that's life, we're still in pre-1.0 and the only thing that changes is the cell paths, not the values. The new deeply nested style would also play nicely with the new table -e viewer.

The important part for me is that both old and new styles are working for at least one release which already seems to be the case in this PR. In the deprecation message you don't need to tie it to 0.80. We could remove the old style as soon as 0.73.

[webbedspace]

Well, I think the changeover couldn't hurt to get at least a couple months leeway. The support footprint in config.rs really isn't that invasive...

[rgwood]

Removed the v0.80 mention because I don't think we want to tie ourselves to a specific timeline for removal. Will merge when CI is green.

Thanks for this change, it's a big+good one!

[fdncred]

Why am I getting this error if it's not a breaking change?

$env.config.table_index_mode is an unknown config setting

It kind of makes me wonder if any others were missed.

[webbedspace]

Urgghh… my dignity…

If that's the only error then that should be the only one missing (the "errors" in config.rs are actually just eprintln!s that don't even stop control flow).

[Kangaxx-0]

@fdncred @rgwood @webbedspace I saw this change did not reorganize Config struct and also its Default impl, are we going to have a further refactor change, or they will stay as is for now(no plan), the inconsistency takes a little more time to find the prop in both places, so i am wondering

[webbedspace]

I'll do it once I implement #7059. (Note: this comment is not legally binding.)

[Kangaxx-0]

I've been working an issue which is to re-assign env.config, the tricky part I am facing right now is I have to do some conversions.

During evaluation, I need to convert value of Int to config supported type, e.g = $env.config.history.max_size = 2000, in this case, 2000 needs to be converted to Record. I could wait until you complete #7059 which could make my change less dramatic :)

[Kangaxx-0]

I'd like to share a little more details for this and some potetional changes we will make. JT created issue #7110 which expects ENV config can be mutated. My local changes parically works, but it is not a clean solution, so I do not feel comfortable enough to create a pr for it. The problem like I said is - right now, the config changes done by this pr and Config in state are inconsistent, some conversions are needed to make assignment works. $env.config.history.max_size = 20000:

20000 is an Int during parse which is correct, but in merge env function call, value's into_config expects the param is Records, also in nu state, the actual mapping config key is max_history_size instead of history.max_size

We need to consider the mutation scenario when working on the next change :) And of course, there are two more issues causing #7110:

• $env.config.xxxx = xxxx create a new Config and replace the old one
• insert the same key to env_vars replace the old one , this is default behavior of HashMap, but it can cause some confusions for end-users

[webbedspace]

$env.config.xxxx = xxxx create a new Config and replace the old one

This is how I understand it "should" work - $env.config.xxxx = xxxx creates a mutable Record from $env.config, performs the assignment onto it, then re-assigns to $env.config. Fairly simple.

[Kangaxx-0]

@webbedspace If you could tag me in your upcoming change, that would be great, we might be able to coordinate and test to get #7110 fixed together