Skip to content

Make which-support feature non-optional#13125

Merged
fdncred merged 3 commits intonushell:mainfrom
IanManske:remove-which-support-feature
Jun 13, 2024
Merged

Make which-support feature non-optional#13125
fdncred merged 3 commits intonushell:mainfrom
IanManske:remove-which-support-feature

Conversation

@IanManske
Copy link
Copy Markdown
Member

Description

Removes the which-support cargo feature and makes all of its feature-gated code enabled by default in all builds. I'm not sure why this one command is gated behind a feature. It seems to be a relic of older code where we had features for what seems like every command.

Copy link
Copy Markdown
Member

@ysthakur ysthakur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job finding this, LGTM but I'll let someone else with more knowledge of cargo approve. Just had one thing.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jun 11, 2024

I asked about this a while back and someone was speculating that the reason it was feature-gated was because of it not working on other operating systems, maybe one of the BSDs but I'm not sure exactly. Maybe @devyn can test on a BSD release to see if there are problems?

@devyn
Copy link
Copy Markdown
Contributor

devyn commented Jun 13, 2024

@fdncred I have not had any trouble compiling or using which-support on FreeBSD, NetBSD or OpenBSD lately. On all three, since mimalloc issues were solved, I've been able to just use the defaults.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jun 13, 2024

ok, let's try it. thanks!

@fdncred fdncred merged commit 634361b into nushell:main Jun 13, 2024
@hustcer hustcer added this to the v0.95.0 milestone Jun 14, 2024
@NotTheDr01ds NotTheDr01ds mentioned this pull request Jun 24, 2024
fdncred pushed a commit that referenced this pull request Jun 29, 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

   ```nu
   > do 1
   Error: nu::shell::cant_convert

     × Can't convert to Closure.
      ╭─[entry #26:1:4]
    1 │ do 1
      ·    ┬
      ·    ╰── can't convert int to Closure
      ╰────
   ```

   ## After

   ```nu
   > do 1
   Error: nu::parser::parse_mismatch

     × Parse mismatch during operation.
      ╭─[entry #5:1:4]
    1 │ do 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
@IanManske IanManske deleted the remove-which-support-feature branch September 1, 2024 22:24
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.

5 participants