Skip to content

fixes a def parsing bug with a default list#8096

Merged
fdncred merged 1 commit intonushell:mainfrom
fdncred:fix_def_list_parsing_bug
Feb 22, 2023
Merged

fixes a def parsing bug with a default list#8096
fdncred merged 1 commit intonushell:mainfrom
fdncred:fix_def_list_parsing_bug

Conversation

@fdncred
Copy link
Copy Markdown
Contributor

@fdncred fdncred commented Feb 16, 2023

Description

This PR fixes a bug where a default list in a custom command parameter wasn't being accepted. The reason was because it was comparing specific types of list like list<any> != list<string>. So, this PR attempts to fix that.

Before

> def f [param: list = [one]] { echo $param }
Error: nu::parser::assignment_mismatch (link)

  × Default value wrong type
   ╭─[entry #1:1:1]
 1 │ def f [param: list = [one]] { echo $param }
   ·                      ──┬──
   ·                        ╰── default value not list<any>
   ╰────

After

> def f [param: list = [one]] {echo $param}
> f
╭───┬─────╮
│ 0 │ one │
╰───┴─────╯

closes #8092

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

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 16, 2023

Codecov Report

Merging #8096 (fbc1894) into main (1fd1a3a) will increase coverage by 1.09%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8096      +/-   ##
==========================================
+ Coverage   53.07%   54.17%   +1.09%     
==========================================
  Files         606      608       +2     
  Lines       98249    99088     +839     
==========================================
+ Hits        52146    53679    +1533     
+ Misses      46103    45409     -694     
Impacted Files Coverage Δ
crates/nu-parser/src/parser.rs 74.15% <0.00%> (+3.08%) ⬆️
crates/nu-protocol/src/ty.rs 84.34% <0.00%> (-2.26%) ⬇️
src/terminal.rs 10.00% <0.00%> (-90.00%) ⬇️
crates/nu-system/src/foreground.rs 61.68% <0.00%> (-35.88%) ⬇️
crates/nu-utils/src/utils.rs 54.66% <0.00%> (-14.83%) ⬇️
crates/nu-command/src/filesystem/cd.rs 19.23% <0.00%> (-7.62%) ⬇️
crates/nu-path/src/tilde.rs 59.67% <0.00%> (-4.12%) ⬇️
crates/nu-test-support/src/playground/play.rs 30.04% <0.00%> (-2.06%) ⬇️
crates/nu-command/src/filesystem/cp.rs 17.45% <0.00%> (-0.54%) ⬇️
crates/nu-command/src/filesystem/rm.rs 24.78% <0.00%> (-0.36%) ⬇️
... and 63 more

@fdncred
Copy link
Copy Markdown
Contributor Author

fdncred commented Feb 16, 2023

I was thinking about this and another way to fix this problem is to allow specific list types to be defined on parameters for custom commands. Such as:

def f [param: list<string> = [one]] { echo $param }

I'm just not sure where to make that change. Seems like some additional parsing may be required somewhere.

@fdncred fdncred added this pull request to the merge queue Feb 22, 2023
Merged via the queue into nushell:main with commit 58829e3 Feb 22, 2023
@fdncred fdncred deleted the fix_def_list_parsing_bug branch February 22, 2023 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot provide default values for list arguments

1 participant