Skip to content
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

Mutation of $env using cell paths doesn't seem to work #7110

Closed
sophiajt opened this issue Nov 12, 2022 · 5 comments · Fixed by #7309 or #7318
Closed

Mutation of $env using cell paths doesn't seem to work #7110

sophiajt opened this issue Nov 12, 2022 · 5 comments · Fixed by #7309 or #7318
Labels
🐛 bug Something isn't working configuration Issue related to nu's configuration
Milestone

Comments

@sophiajt
Copy link
Contributor

Describe the bug

$env.config.max_history_size = 20000 should update the max_history_size in the config field.

This currently doesn't work correctly.

How to reproduce

See above

Expected behavior

$env.config.max_history_size = 20000 should set the deeper value without issue.

Screenshots

No response

Configuration

key value
version 0.71.1
branch main
commit_hash b650d1e
build_os windows-x86_64
build_target x86_64-pc-windows-msvc
rust_version rustc 1.63.0 (4b91a6ea7 2022-08-08)
rust_channel 1.63.0-x86_64-pc-windows-msvc
cargo_version cargo 1.63.0 (fd9c4297c 2022-07-01)
pkg_version 0.71.1
build_time 2022-11-11 21:00:30 +13:00
build_rust_channel release
features database, dataframe, default, trash, which, zip
installed_plugins

Additional context

No response

@sholderbach sholderbach added 🐛 bug Something isn't working configuration Issue related to nu's configuration labels Nov 12, 2022
@webbedspace
Copy link
Contributor

webbedspace commented Nov 16, 2022

Hold on, does any mutation via cell paths actually work?

〉let a = [[[1] [2] [3]] [[1] [2] [3]]]
〉$a
╭───┬────────────────╮
│ 0 │ [list 3 items] │
│ 1 │ [list 3 items] │
╰───┴────────────────╯
〉$a.0.0
╭───┬───╮
│ 0 │ 1 │
╰───┴───╯
〉$a.0.0 = 4
Error: nu::parser::parse_mismatch (link)

  × Parse mismatch during operation.
   ╭─[entry #9:1:1]
 1 │ $a.0.0 = 4
   ·        ┬
   ·        ╰── expected operator
   ╰────

@webbedspace
Copy link
Contributor

UPDATE: Nevermind, I figured it out, you mean with that new mut doohicky, right.

@Kangaxx-0
Copy link
Contributor

Seems like max_history_size is an int which can not be converted to Record

@webbedspace
Copy link
Contributor

Ignore the specifics of the example in the top post; the config structure changed recently so it's not necessarily still valid.

@Kangaxx-0
Copy link
Contributor

I do not think the recent config refactoring is the root cause of this issue, taking $env.config.max_history_size = 20000 for example, the part of 20000 is an Int, but merge_env expects all configs to be Records, also, the evaluation phase is not handling Assign operator 100% correct. I will bring up this to discord for further discussion.

kubouch pushed a commit that referenced this issue Dec 6, 2022
…7318)

# Description

Closes #7110. ~~Note that unlike "real" `mut` vars, $env can be deeply
mutated via stuff like `$env.PYTHON_IO_ENCODING = utf8` or
`$env.config.history.max_size = 2000`. So, it's a slightly awkward
special case, arguably justifiable because of what $env represents (the
environment variables of your system, which is essentially "outside"
normal Nushell regulations).~~
EDIT: Now allows all `mut` vars to be deeply mutated using `=`, on
request.

# User-Facing Changes

See above.

# Tests + Formatting

Don't forget to add tests that cover your changes.

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 -- -D warnings -D clippy::unwrap_used -A
clippy::needless_collect` to check that you're using the standard code
style
- `cargo test --workspace` to check that all tests pass

# After Submitting

If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
@hustcer hustcer added this to the v0.73 milestone Dec 7, 2022
kubouch pushed a commit that referenced this issue Dec 10, 2022
# Description
Closes #7059. Rather than generate a new Record each time $env.config is
accessed (as described in that issue), instead `$env.config = ` now A)
parses the input record, then B) un-parses it into a clean Record with
only the valid values, and stores that as an env-var. The reasoning for
this is that I believe `config_to_nu_record()` (the method that performs
step B) will be useful in later PRs. (See below)

As a result, this also "fixes" the following "bug":
```
〉$env.config = 'butts'
$env.config is not a record
〉$env.config
butts
```
~~Instead, `$env.config = 'butts'` now turns `$env.config` into the
default (not the default config.nu, but `Config::default()`, which
notably has empty keybindings, color_config, menus and hooks vecs).~~

This doesn't attempt to fix #7110. cc @Kangaxx-0

# Example of new behaviour

OLD:
```
〉$env.config = ($env.config | merge { foo: 1 })
$env.config.foo is an unknown config setting
〉$env.config.foo
1
```
NEW:
```
〉$env.config = ($env.config | merge { foo: 1 })
Error:
  × Config record contains invalid values or unknown settings

Error:
  × Error while applying config changes
   ╭─[entry #1:1:1]
 1 │ $env.config = ($env.config | merge { foo: 1 })
   ·                                           ┬
   ·                                           ╰── $env.config.foo is an unknown config setting
   ╰────
  help: This value has been removed from your $env.config record.

〉$env.config.foo
Error: nu:🐚:column_not_found (link)

  × Cannot find column
   ╭─[entry #1:1:1]
 1 │ $env.config = ($env.config | merge { foo: 1 })
   ·                              ──┬──
   ·                                ╰── value originates here
   ╰────
   ╭─[entry #2:1:1]
 1 │ $env.config.foo
   ·             ─┬─
   ·              ╰── cannot find column 'foo'
   ╰────
```
# Example of new errors

OLD:
```
$env.config.cd.baz is an unknown config setting
$env.config.foo is an unknown config setting
$env.config.bar is an unknown config setting
$env.config.table.qux is an unknown config setting
$env.config.history.qux is an unknown config setting
```
NEW:
```
Error: 
  × Config record contains invalid values or unknown settings

Error:
  × Error while applying config changes
     ╭─[C:\Users\Leon\AppData\Roaming\nushell\config.nu:267:1]
 267 │     abbreviations: true # allows `cd s/o/f` to expand to `cd some/other/folder`
 268 │     baz: 3,
     ·          ┬
     ·          ╰── $env.config.cd.baz is an unknown config setting
 269 │   }
     ╰────
  help: This value has been removed from your $env.config record.

Error:
  × Error while applying config changes
     ╭─[C:\Users\Leon\AppData\Roaming\nushell\config.nu:269:1]
 269 │   }
 270 │   foo: 1,
     ·        ┬
     ·        ╰── $env.config.foo is an unknown config setting
 271 │   bar: 2,
     ╰────
  help: This value has been removed from your $env.config record.

Error:
  × Error while applying config changes
     ╭─[C:\Users\Leon\AppData\Roaming\nushell\config.nu:270:1]
 270 │   foo: 1,
 271 │   bar: 2,
     ·        ┬
     ·        ╰── $env.config.bar is an unknown config setting
     ╰────
  help: This value has been removed from your $env.config record.

Error:
  × Error while applying config changes
     ╭─[C:\Users\Leon\AppData\Roaming\nushell\config.nu:279:1]
 279 │     }
 280 │     qux: 4,
     ·          ┬
     ·          ╰── $env.config.table.qux is an unknown config setting
 281 │   }
     ╰────
  help: This value has been removed from your $env.config record.

Error:
  × Error while applying config changes
     ╭─[C:\Users\Leon\AppData\Roaming\nushell\config.nu:285:1]
 285 │     file_format: "plaintext" # "sqlite" or "plaintext"
 286 │  qux: 2
     ·       ┬
     ·       ╰── $env.config.history.qux is an unknown config setting
 287 │   }
     ╰────
  help: This value has been removed from your $env.config record.


```

# User-Facing Changes

See above.

# Tests + Formatting

Don't forget to add tests that cover your changes.

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 -- -D warnings -D clippy::unwrap_used -A
clippy::needless_collect` to check that you're using the standard code
style
- `cargo test --workspace` to check that all tests pass

# After Submitting

If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working configuration Issue related to nu's configuration
Projects
None yet
5 participants