Make `get` hole errors and cell path hole errors identical (improvement on #7002) by webbedspace · Pull Request #7647 · nushell/nushell

webbedspace · 2023-01-01T08:29:24Z

Description

This closes #7498, as well as fixes an issue reported in #7002 (comment)

BEFORE:

〉[{foo: 'bar'} {}] | get foo
Error: nu::shell::column_not_found (link)

  × Cannot find column
   ╭─[entry #5:1:1]
 1 │ [{foo: 'bar'} {}] | get foo
   · ────────┬────────   ─┬─
   ·         │            ╰── value originates here
   ·         ╰── cannot find column 'Empty cell'
   ╰────

〉[{foo: 'bar'} {}].foo
╭───┬─────╮
│ 0 │ bar │
│ 1 │     │
╰───┴─────╯

AFTER:

〉[{foo: 'bar'} {}] | get foo
Error: nu::shell::column_not_found (link)

  × Cannot find column
   ╭─[entry #1:1:1]
 1 │ [{foo: 'bar'} {}] | get foo
   ·               ─┬        ─┬─
   ·                │         ╰── cannot find column 'foo'
   ·                ╰── value originates here
   ╰────

〉[{foo: 'bar'} {}].foo
Error: nu::shell::column_not_found (link)

  × Cannot find column
   ╭─[entry #3:1:1]
 1 │ [{foo: 'bar'} {}].foo
   ·               ─┬  ─┬─
   ·                │   ╰── cannot find column 'foo'
   ·                ╰── value originates here       
   ╰────

EDIT: This also changes the semantics of get/select -i somewhat. I've decided to leave it like this because it works more intuitively with default and compact.
BEFORE:

〉[{a:1} {b:2} {a:3}] | select -i foo | to nuon
null

AFTER:

〉[{a:1} {b:2} {a:3}] | select -i foo | to nuon
[[foo]; [null], [null], [null]]

User-Facing Changes

See above. EDIT: the issue with holes in cases like [{foo: 'bar'} {}].foo.0 versus [{foo: 'bar'} {}].0.foo has been resolved.

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 after the PR is merged, if necessary. This will help us keep the docs up to date.

rgwood · 2023-01-01T09:04:03Z

Nice!

I think this is a step in the right direction. We may also need to adjust the behaviour of get -i (not saying it has to happen in this PR):

〉[{ foo: "bar"} {}] | get -i foo
〉[{ foo: "bar"} {}] | get -i foo | to nuon
null
〉[{ foo: "bar"} {}] | select -i foo
╭───┬─────╮
│ # │ foo │
├───┼─────┤
│ 0 │ bar │
│ 1 │     │
╰───┴─────╯

IMO the behaviour of get -i where it returns a single null value is not very useful; it should behave like select and replace missing values with nulls. That's how I envision cell paths working when users opt into lenient error checking (with -i or some future ? syntax); curious whether other people agree.

webbedspace · 2023-01-01T11:04:31Z

I came up with an ingenious way of making [{foo: 'bar'} {}].foo.0 still work, even though the .foo access should produce an error at that point in cell path evaluation. See here for details. This method has the side-effect that stuff like [{foo: 'bar'} {}].foo | reject 1 also works without erroring.

rgwood · 2023-01-01T19:05:28Z

Hmm, I think that change is generally a good idea but get -i seems more broken now:

〉[{ foo: "bar"} {}] | get -i foo
Error: nu::shell::column_not_found (link)

  × Cannot find column
   ╭─[entry #7:1:1]
 1 │ [{ foo: "bar"} {}] | get -i foo
   ·                ─┬           ─┬─
   ·                 │            ╰── cannot find column 'foo'
   ·                 ╰── value originates here
   ╰────

webbedspace · 2023-01-02T09:24:27Z

OK, I fixed it. However, I just noticed I've inadvertently changed the semantics of -i somewhat.

BEFORE:

〉[{a:1} {b:2} {a:3}] | select -i a | to nuon
[[a]; [1], [null], [3]]
〉[{a:1} {b:2} {a:3}] | select -i foo | to nuon
null

AFTER:

〉[{a:1} {b:2} {a:3}] | select -i a | to nuon
[[a]; [1], [null], [3]]
〉[{a:1} {b:2} {a:3}] | select -i foo | to nuon
[[foo]; [null], [null], [null]]

I've decided that the latter way is better because it can interact with default more intuitively:

〉[{a:1} {b:2} {a:3}] | select -i a | default 0 a
[[a]; [1], [0], [3]]
〉[{a:1} {b:2} {a:3}] | select -i foo | default 0 foo
[[foo]; [0], [0], [0]]

webbedspace · 2023-01-02T19:06:40Z

crates/nu-command/tests/commands/default.rs

                open los_tres_amigos.json
                | get amigos
-                | default rusty_luck 1
+                | default 1 rusty_luck


By the way… am I losing my mind, or was this test always wrong until now? default rusty_luck 1 is not the correct order of default arguments, and never was, right?

Suspect the test was always wrong.

FWIW I also assumed that the order was default <column name> <default value> instead of when I first encountered default, I find the current order somewhat unintuitive.

webbedspace · 2023-01-02T19:09:40Z

crates/nu-protocol/src/pipeline_data.rs

                let working_set = StateWorkingSet::new(engine_state);
-
+                // Value::Errors must always go to stderr, not stdout.
+                is_err = true;


Does anyone have an opinion about these lines? I don't know why Value::Errors seemingly were allowed to go to stdout, but that seemed wrong to me (and I needed to make this change to make a test pass, due to the aforementioned business about creating but not returning Value::Errors in follow_cell_path_helper().)

I think it makes sense to force errors to stderr.

rgwood · 2023-01-02T19:47:47Z

OK, I fixed it. However, I just noticed I've inadvertently changed the semantics of -i somewhat.

Sounds good to me - the "-i sometimes collapses everything into a single null" behaviour was bad IMO.

rgwood · 2023-01-02T20:21:49Z

crates/nu-command/src/filters/length.rs

-        _ => Ok(Value::int(input.into_iter().count() as i64, call.head).into_pipeline_data()),
+        _ => {
+            let mut count: i64 = 0;
+            // Check for and propagate errors


I'm a little concerned by this approach of checking for errors in downstream commands.I worry that there will be a long tail of commands where we forget to do it, leading to inconsistency. It also takes us further away from an "errors are just data" approach (which we admittedly do not adhere to very well today).

That said, users who do something like $foo | get bar | length will expect it to fail on an unsuccessful bar access... so maybe this is an OK approach for now.

In the future it would be nice if this sort of check was handled at a higher level (not sure exactly where).

rgwood

Thanks for the PR!

I think this is a good incremental improvement to cell paths. I really like that behaviour is more consistent across get and select now, and that -i no longer collapses multiple errors to a single null value.

This one will probably break a handful of scripts, and it will need an explanation in the 0.74 blog post (0.74 is being released on Jan 10): nushell/nushell.github.io#715

I'm happy to merge this, just want to get another opinion first.

fdncred · 2023-01-02T22:40:48Z

+1 for more consistent and expected behavior

Make get hole errors and cell path hole errors identical.

fd7257b

webbedspace added 2 commits January 1, 2023 20:56

Put errors in lists instead of returning them

f1d096e

Clippy

4e7bd61

Clippy

d6d3fb0

webbedspace force-pushed the hole-error branch from a9dafcf to 322224d Compare January 2, 2023 09:29

Fix get/select -i and improve semantics

b81dfe9

webbedspace force-pushed the hole-error branch from 322224d to b81dfe9 Compare January 2, 2023 09:29

webbedspace commented Jan 2, 2023

View reviewed changes

rgwood added the notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes label Jan 2, 2023

rgwood reviewed Jan 2, 2023

View reviewed changes

rgwood approved these changes Jan 2, 2023

View reviewed changes

rgwood merged commit 65d0b5b into nushell:main Jan 2, 2023

webbedspace deleted the hole-error branch January 4, 2023 08:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `get` hole errors and cell path hole errors identical (improvement on #7002)#7647

Make `get` hole errors and cell path hole errors identical (improvement on #7002)#7647
rgwood merged 5 commits intonushell:mainfrom
webbedspace:hole-error

webbedspace commented Jan 1, 2023 •

edited

Loading

Uh oh!

rgwood commented Jan 1, 2023 •

edited

Loading

Uh oh!

webbedspace commented Jan 1, 2023 •

edited

Loading

Uh oh!

rgwood commented Jan 1, 2023

Uh oh!

webbedspace commented Jan 2, 2023

Uh oh!

webbedspace Jan 2, 2023

Uh oh!

rgwood Jan 2, 2023

Uh oh!

webbedspace Jan 2, 2023 •

edited

Loading

Uh oh!

rgwood Jan 2, 2023

Uh oh!

rgwood commented Jan 2, 2023 •

edited

Loading

Uh oh!

rgwood Jan 2, 2023

Uh oh!

rgwood left a comment •

edited

Loading

Uh oh!

fdncred commented Jan 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

webbedspace commented Jan 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

User-Facing Changes

Tests + Formatting

After Submitting

Uh oh!

rgwood commented Jan 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webbedspace commented Jan 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgwood commented Jan 1, 2023

Uh oh!

webbedspace commented Jan 2, 2023

Uh oh!

webbedspace Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

rgwood Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

webbedspace Jan 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgwood Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

rgwood commented Jan 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgwood Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

rgwood left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fdncred commented Jan 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

webbedspace commented Jan 1, 2023 •

edited

Loading

rgwood commented Jan 1, 2023 •

edited

Loading

webbedspace commented Jan 1, 2023 •

edited

Loading

webbedspace Jan 2, 2023 •

edited

Loading

rgwood commented Jan 2, 2023 •

edited

Loading

rgwood left a comment •

edited

Loading