Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow string to copmpare with another string #11590

Conversation

yukitomoda
Copy link
Contributor

Description

Nushell parser now reject comparison operator with 2 strings (e.g. "abc" < "cba"). This pr fixes it.

before

~
❯ "abc" < "bca"
Error: nu::parser::unsupported_operation

  × less-than comparison is not supported on values of type string
   ╭─[entry #43:1:1]
 1"abc" < "bca"
   · ──┬── ┬
   ·   │   ╰── doesn't support this value
   ·   ╰── string
   ╰────


~
❯ def foo []: nothing -> string { "abc" }

~
❯ (foo) < "bca"
Error: nu::parser::unsupported_operation

  × less-than comparison is not supported on values of type string
   ╭─[entry #53:1:1]
 1 │ (foo) < "bca"
   · ──┬── ┬
   ·   │   ╰── doesn't support this value
   ·   ╰── string
   ╰────

after

~
❯ "abc" < "bca"
true

~
❯ def foo []: nothing -> string { "abc" }

~
❯ (foo) < "bca"
true

User-Facing Changes

Following pattern will be allowed.

operator type of lhs type of rhs result
< string string bool
<= string string bool
> string string bool
>= string string bool

Tests + Formatting

  • cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
  • cargo clippy --workspace -- -D warnings -D clippy::unwrap_used to check that you're using the standard code style
  • cargo test --workspace to check that all tests pass (on Windows make sure to enable developer mode)
  • cargo run -- -c "use std testing; testing run-tests --path crates/nu-std" to run the tests for the standard library

After Submitting

@yukitomoda yukitomoda changed the title Allow string constant to copmpare with another string Allow string to copmpare with another string Jan 20, 2024
@fdncred
Copy link
Collaborator

fdncred commented Jan 20, 2024

please forgive my ignorance but i've never really understood this. What does "abc" < "bca" even mean? What exactly is it comparing to say this is true?

@yukitomoda
Copy link
Contributor Author

please forgive my ignorance but i've never really understood this. What does "abc" < "bca" even mean? What exactly is it comparing to say this is true?

"abc" and "bca" are compared by using std::string::String::partial_cmp internally. This compares the strings' UTF-8 representation as Vec<u8>.
This is the same behavior as comparing strings statically typed as any which was also allowed in Nushell up to now (e.g. (do { "abc" }) < "bca"). So I think it is consistent.

@fdncred
Copy link
Collaborator

fdncred commented Jan 21, 2024

ok, thanks for the explanation.

@fdncred fdncred merged commit 6edf91d into nushell:main Jan 21, 2024
19 checks passed
@yukitomoda yukitomoda deleted the fix-parse-copmarison-operator-with-string-and-string branch January 21, 2024 14:25
@hustcer hustcer added this to the v0.90.0 milestone Feb 3, 2024
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this pull request Feb 20, 2024
# Description

Nushell parser now reject comparison operator with 2 strings (e.g.
`"abc" < "cba"`). This pr fixes it.

## before

```nu
~
❯ "abc" < "bca"
Error: nu::parser::unsupported_operation

  × less-than comparison is not supported on values of type string
   ╭─[entry nushell#43:1:1]
 1 │ "abc" < "bca"
   · ──┬── ┬
   ·   │   ╰── doesn't support this value
   ·   ╰── string
   ╰────


~
❯ def foo []: nothing -> string { "abc" }

~
❯ (foo) < "bca"
Error: nu::parser::unsupported_operation

  × less-than comparison is not supported on values of type string
   ╭─[entry nushell#53:1:1]
 1 │ (foo) < "bca"
   · ──┬── ┬
   ·   │   ╰── doesn't support this value
   ·   ╰── string
   ╰────
```

## after

```nu
~
❯ "abc" < "bca"
true

~
❯ def foo []: nothing -> string { "abc" }

~
❯ (foo) < "bca"
true
```

# User-Facing Changes

Following pattern will be allowed.

| operator | type of lhs | type of rhs | result |
| -------- | ----------- | ----------- | ------ |
| `<`      | string      | string      | bool   |
| `<=`     | string      | string      | bool   |
| `>`      | string      | string      | bool   |
| `>=`     | string      | string      | bool   |

# Tests + Formatting

- [x] `cargo fmt --all -- --check` to check standard code formatting
(`cargo fmt --all` applies these changes)
- [x] `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used`
to check that you're using the standard code style
- [x] `cargo test --workspace` to check that all tests pass (on Windows
make sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- [x] `cargo run -- -c "use std testing; testing run-tests --path
crates/nu-std"` to run the tests for the standard library

# After Submitting
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.

None yet

3 participants