Skip to content

Fix do signature#13216

Merged
fdncred merged 1 commit intonushell:mainfrom
NotTheDr01ds:fix-do-signature
Jun 29, 2024
Merged

Fix do signature#13216
fdncred merged 1 commit intonushell:mainfrom
NotTheDr01ds:fix-do-signature

Conversation

@NotTheDr01ds
Copy link
Copy Markdown
Contributor

@NotTheDr01ds NotTheDr01ds commented Jun 24, 2024

Recommend holding until after #13125 is fully digested and possibly until 0.96.

Description

Fixes one of the issues described in #13125

The do signature included a SyntaxShape:Any as one of the possible first-positional types. This is incorrect. do only takes a closure as a positional. This had the result of:

  1. Moving what should have been a parser error to evaluation-time

    Before

    > do 1
    Error: nu::shell::cant_convert
    
      × Can't convert to Closure.
       ╭─[entry #26:1:4]
     1do 1
       ·    ┬
       ·    ╰── can't convert int to Closure
       ╰────

    After

    > do 1
    Error: nu::parser::parse_mismatch
    
      × Parse mismatch during operation.
       ╭─[entry #5:1:4]
     1do 1
       ·    ┬
       ·    ╰── expected block, closure or record
       ╰────
  2. Masking a bad test in std assert

This is a bit convoluted, but std assert tests included testing assert error to make sure it:

  • Asserts on bad code
  • Doesn't assert on good code

The good-code test was broken, and was essentially bad-code (really bad-code) that wasn't getting caught due to the bad signature.

Fixing this resulted in parse time failures on every call to test_asserts (not something that particular test was designed to handle.

This PR also fixes the test case to properly evaluate std assert error against a good code path.

User-Facing Changes

  • Error-type returned (possible breaking change?)

Tests + Formatting

  • 🟢 toolkit fmt
  • 🟢 toolkit clippy
  • 🟢 toolkit test
  • 🟢 toolkit test stdlib

After Submitting

N/A

@NotTheDr01ds NotTheDr01ds changed the title Fixed do signature Fix do signature Jun 24, 2024
@fdncred fdncred merged commit a287333 into nushell:main Jun 29, 2024
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jun 29, 2024

Thanks

@hustcer hustcer added this to the v0.96.0 milestone Jun 30, 2024
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.

3 participants