last, skip, drop, take until, take while, skip until, skip while, where, reverse, shuffle, append, prepend and sort-by raise error when given non-lists #7623

WindSoilder · 2022-12-28T11:32:06Z

Description

Closes: #6941

About the change

It seems that all these command need to check if input is a list, to avoid duplicate code, make a new method called into_iter_strict in PipelineData, so we just need to change from into_iter to into_iter_restrict.

It's slightly different to #7123, which has less overhead(especially if the input type is Binary), but I think it's ok because we have less code

User-Facing Changes

❯ 1 | last
Error: nu::shell::only_supports_this_input_type (link)

  × Only supports for specific types.
   ╭─[entry #1:1:1]
 1 │ 1 | last
   · ┬   ──┬─
   · │     ╰── only list, binary, exernal stream or range input data is supported
   · ╰── input type: int
   ╰────

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.

crates/nu-protocol/src/pipeline_data.rs

crates/nu-protocol/src/shell_error.rs

webbedspace · 2022-12-29T13:26:15Z

crates/nu-protocol/src/shell_error.rs

-    #[error("Pipeline mismatch.")]
-    #[diagnostic(code(nu::shell::pipeline_mismatch), url(docsrs))]
+    #[error("Only supports for specific types.")]
+    #[diagnostic(code(nu::shell::only_supports_this_input_type), url(docsrs))]


The #[diagnostic] trait creates a doc link, but only_supports_this_input_type isn't in the docs, is it? Maybe go over there and add it.

I think it's fine, that will be added when the next version of nushell is published

rgwood · 2023-01-01T02:18:31Z

Thank you for the PR! I noticed that after this change, PATH configuration in my env.nu started breaking:

Error: nu::shell::only_supports_this_input_type (link)

  × Only supports for specific input types.
    ╭─[Host Environment Variables:29:1]
 29 │ "NUSHELL_CURRENT_SHELL"="0"
 30 │ "PATH"="/home/linuxbrew/.linuxbrew/bin:/opt/homebrew/bin:/usr

(long PATH string truncated)

                         ╰── input type: string
 31 │ "PULSE_SERVER"="/mnt/wslg/PulseServer"
    ╰────
    ╭─[/home/reilly/.config/nushell/env.nu:61:1]
    ·                             ───┬───
 63 │ 
    ╰────

The problem was that I had some lines like this:

let-env PATH = ($env.PATH | append '/some/path')

Instead of this:

let-env PATH = ($env.PATH | split row (char esep) | append '/some/path' )

We still recommend the former approach in the docs, so I believe many users will be impacted by this.

Not sure what the right approach is, but at a minimum we should update the docs and mention this prominently in the release post.

A tiny follow-up from #7623, changes "Only supports for specific input types" to "Input type not supported" Before: ``` 〉"asdf" | append "foo" Error: nu::shell::only_supports_this_input_type (link) × Only supports for specific input types. ╭─[entry #2:1:1] 1 │ "asdf" | append "foo" · ───┬── ───┬── · │ ╰── only list, binary, raw data or range input data is supported · ╰── input type: string ╰──── ``` After: ``` 〉"asdf" | append "foo" Error: nu:🐚:only_supports_this_input_type (link) × Input type not supported. ╭─[entry #2:1:1] 1 │ "asdf" | append "foo" · ───┬── ───┬── · │ ╰── only list, binary, raw data or range input data is supported · ╰── input type: string ╰──── ```

kubouch · 2023-01-01T10:23:12Z

@rgwood let-env PATH = ($env.PATH | append '/some/path') in env.nu has been always wrong because incoming environment variables in env.nu are still strings, they have not been converted to Values yet. Should be fixed.

fdncred · 2023-01-01T13:54:31Z

Aggravating. I had the same problem and had to git bisect to get here.

WindSoilder · 2023-01-01T15:44:25Z

Hmm... Should we revert the pr or make prepend and append comand works as old ways?

fdncred · 2023-01-01T18:24:03Z

@WindSoilder, Nah. I don't think so. But we do need to say something in the change log with an example like I gave in the status-update discord channel.

rgwood · 2023-01-01T18:56:33Z

let-env PATH = ($env.PATH | split row (char esep) | append '/some/path' ) feels a lot less intuitive than let-env PATH = ($env.PATH | append '/some/path'), even if it was only an accident that it worked without the split row (char esep). It's an unfortunate UX regression, but I don't have any ideas for how to avoid it.

sholderbach · 2023-01-01T20:20:30Z

let-env PATH = ($env.PATH | split row (char esep) | append '/some/path' ) feels a lot less intuitive than let-env PATH = ($env.PATH | append '/some/path'), even if it was only an accident that it worked without the split row (char esep). It's an unfortunate UX regression, but I don't have any ideas for how to avoid it.

Yeah that should not be how path handling in nushell is done. Either we provide special path handling sugar for such a common usecase or we have to get some escape hatch back.

fdncred · 2023-01-01T20:53:25Z

I think it's different than we're used to but, IMO, now that I've thought about it, it's more accurate since the path env var is just a string at this point, you should have to split it before appending/prepending.

Having said that, I'm not against some easier way to do it. It's always been a bit kludgy to me to update the path this way.

Ideas:

$env.PATH += '/some/path' <-- would probably have to add something to += operator
$env.PATH += $'(char esep)/some/path' <-- i think? this should work now
$env.PATH | prepend --delimiter (char esep) '/some/path' <-- may have to tweak append or prepend
$env.PATH | prepend $'(char esep)/some/path' <-- probably needs tweaking
$env.PATH | config path-prepend '/some/path' <-- new command(s)
?

rgwood · 2023-01-01T21:01:05Z

$env.PATH | prepend '/some/path' but it gets run after the PATH has been converted to a Value (not sure exactly where this would go...)

sholderbach · 2023-01-01T21:03:43Z

Butterfly meme is this operator overloading?

fdncred · 2023-01-01T21:08:55Z

@rgwood does your option 6 work only with $env.PATH or does this change the way prepend and append work with all strings?

rgwood · 2023-01-01T21:10:34Z

Neither - I was thinking the user puts $env.PATH | prepend '/some/path' in a place where Nu knows to execute it after ENV_CONVERSIONS has been run.

fdncred · 2023-01-01T21:12:17Z

Ok. Understood. I think that means that users put it in config.nu since env.nu runs before config.nu.

kubouch · 2023-01-02T07:53:51Z

You can also do $env.PATH += $'(char esep)/some/path' and let ENV_CONVERSIONS handle the splitting...

Ok. Understood. I think that means that users put it in config.nu since env.nu runs before config.nu.
@fdncred ENV_CONVERSIONS are done after config.nu

That being said, I've been thinking the ENV_CONVERSION system is not particularly elegant and actually not used much except a couple of cases (PATH, but I use it also for LD_LIBRARY_PATH and similar delimited env vars). Maybe there is a better way to handle all of this.

fdncred · 2023-01-02T14:25:48Z

@fdncred ENV_CONVERSIONS are done after config.nu

Interesting. I find it confusing that ENV_CONVERSIONS are created in env.nu but only done after config.nu.

rgwood · 2023-01-02T17:36:56Z

I would vote to temporarily revert the change for append and prepend. Not sure we're going to land on a better solution before the next release (only 8 days away).

WindSoilder · 2023-01-03T00:42:57Z

I would vote to temporarily revert the change for append and prepend. Not sure we're going to land on a better solution before the next release (only 8 days away).

Yeah I agree with that

# Description #7623 causes a break on PATH convertion, this pr is going to revert `prepend` and `append` bahavior. # User-Facing Changes _(List of all changes that impact the user experience here. This helps us keep track of breaking changes.)_ # 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.

webbedspace · 2023-01-17T11:44:01Z

Any news about this? Personally, I've been thinking that for something as fundamental as PATH/Path, the default ENV_CONVERSIONS should be baked into the Nushell executable as defaults used whenever ENV_CONVERSIONS is unset, and an explicit ENV_CONVERSIONS setting (like in the default env.nu) can override it, similar to how various color_config options have overrideable defaults.

kubouch · 2023-01-17T13:33:20Z

Yeah, we're currently looking into refactoring the config system, including PATH and ENV_CONVERSIONS handling to make it more predictable and user-friendly.

WindSoilder added 5 commits December 28, 2022 17:48

introduce PipelineData::into_iter_strict method

23ac648

fix tests

7c04b10

add tests for new behavior

d132bab

fix err msg

42e6175

fix test

942bc58

webbedspace reviewed Dec 29, 2022

View reviewed changes

crates/nu-protocol/src/pipeline_data.rs Outdated Show resolved Hide resolved

webbedspace reviewed Dec 29, 2022

View reviewed changes

crates/nu-protocol/src/shell_error.rs Outdated Show resolved Hide resolved

webbedspace reviewed Dec 29, 2022

View reviewed changes

WindSoilder added 2 commits December 30, 2022 16:48

make some little fixes

4f0b1cc

fix tests

721064c

sholderbach added the pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes label Dec 30, 2022

kubouch merged commit e9cc417 into nushell:main Dec 31, 2022

WindSoilder deleted the iterator_restrict branch January 1, 2023 03:00

rgwood mentioned this pull request Jan 1, 2023

Tweak new input type error message #7646

Merged

WindSoilder mentioned this pull request Jan 3, 2023

revert changes on prepend and append #7660

Merged

rgwood mentioned this pull request Jan 3, 2023

first, take, last, skip, drop, take until, take while, skip until, skip while, where, reverse, shuffle, append, prepend and sort-by should error when given non-lists instead of coercing to list #6941

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

last, skip, drop, take until, take while, skip until, skip while, where, reverse, shuffle, append, prepend and sort-by raise error when given non-lists #7623

last, skip, drop, take until, take while, skip until, skip while, where, reverse, shuffle, append, prepend and sort-by raise error when given non-lists #7623

WindSoilder commented Dec 28, 2022 •

edited

Loading

webbedspace Dec 29, 2022 •

edited

Loading

WindSoilder Dec 30, 2022

rgwood commented Jan 1, 2023

kubouch commented Jan 1, 2023 •

edited

Loading

fdncred commented Jan 1, 2023

WindSoilder commented Jan 1, 2023

fdncred commented Jan 1, 2023

rgwood commented Jan 1, 2023

sholderbach commented Jan 1, 2023

fdncred commented Jan 1, 2023 •

edited

Loading

rgwood commented Jan 1, 2023

sholderbach commented Jan 1, 2023

fdncred commented Jan 1, 2023

rgwood commented Jan 1, 2023

fdncred commented Jan 1, 2023

kubouch commented Jan 2, 2023

fdncred commented Jan 2, 2023 •

edited

Loading

rgwood commented Jan 2, 2023 •

edited

Loading

WindSoilder commented Jan 3, 2023

webbedspace commented Jan 17, 2023 •

edited

Loading

kubouch commented Jan 17, 2023

last, skip, drop, take until, take while, skip until, skip while, where, reverse, shuffle, append, prepend and sort-by raise error when given non-lists #7623

last, skip, drop, take until, take while, skip until, skip while, where, reverse, shuffle, append, prepend and sort-by raise error when given non-lists #7623

Conversation

WindSoilder commented Dec 28, 2022 • edited Loading

Description

About the change

User-Facing Changes

Tests + Formatting

After Submitting

webbedspace Dec 29, 2022 • edited Loading

Choose a reason for hiding this comment

WindSoilder Dec 30, 2022

Choose a reason for hiding this comment

rgwood commented Jan 1, 2023

kubouch commented Jan 1, 2023 • edited Loading

fdncred commented Jan 1, 2023

WindSoilder commented Jan 1, 2023

fdncred commented Jan 1, 2023

rgwood commented Jan 1, 2023

sholderbach commented Jan 1, 2023

fdncred commented Jan 1, 2023 • edited Loading

rgwood commented Jan 1, 2023

sholderbach commented Jan 1, 2023

fdncred commented Jan 1, 2023

rgwood commented Jan 1, 2023

fdncred commented Jan 1, 2023

kubouch commented Jan 2, 2023

fdncred commented Jan 2, 2023 • edited Loading

rgwood commented Jan 2, 2023 • edited Loading

WindSoilder commented Jan 3, 2023

webbedspace commented Jan 17, 2023 • edited Loading

kubouch commented Jan 17, 2023

WindSoilder commented Dec 28, 2022 •

edited

Loading

webbedspace Dec 29, 2022 •

edited

Loading

kubouch commented Jan 1, 2023 •

edited

Loading

fdncred commented Jan 1, 2023 •

edited

Loading

fdncred commented Jan 2, 2023 •

edited

Loading

rgwood commented Jan 2, 2023 •

edited

Loading

webbedspace commented Jan 17, 2023 •

edited

Loading