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

Change range semantic to behaves more like rust. #12890

Closed
wants to merge 9 commits into from

Conversation

WindSoilder
Copy link
Collaborator

@WindSoilder WindSoilder commented May 17, 2024

Description

We have decided to make range to behave like rust, that is:

> 1..3
╭───┬───╮
│ 0 │ 1 │
│ 1 │ 2 │
╰───┴───╯
> 1..=3
╭───┬───╮
│ 0 │ 1 │
│ 1 │ 2 │
│ 2 │ 3 │
╰───┴───╯
> 1..<3
Error: nu::shell::external_command

  × External command failed
   ╭─[entry #2:1:1]
 1 │ 1..<3
   · ──┬──
   ·   ╰── executable was not found
   ╰────
  help: No such file or directory (os error 2)

This pr also:

  1. removes 1..<3 syntax. The main change is happened in crates/nu-parser/src/parser.rs:parse_range
  2. change range semantic in these commands due to changes in crates/nu-cmd-base/src/util.rs
  • detect_columns (for -c flag)
  • str substring
  • str index-of (for -r flag)
  • bytes at
    Take str substring as example:

Before

❯ "abcd" | str substring 1..=3
bc

After

❯ "abcd" | str substring 1..=3
bcd

The result of "abcd" | str substring 1..3 remains unchanged because the semantic of 1..3 is changed from [1,2,3] to [1,2], so it returns "bc".

Apart from changes in crates/nu-parser/src/parser.rs:parse_range and crates/nu-cmd-base/src/util.rs, remaining changes are most about adjusting test code to follow new semantic

Fixes: #7761

User-Facing Changes

It's a big breaking change, because the semantic of 1..3 is changed from [1,2,3] to [1,2]

Tests + Formatting

Adjusted all tests

After Submitting

Need to update documentation about ranges

@WindSoilder WindSoilder added pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes pr:release-note-mention Addition/Improvement to be mentioned in the release notes pr:language This PR makes some language changes labels May 17, 2024
@WindSoilder WindSoilder added the pr:commands This PR changes our commands in some way label May 17, 2024
@WindSoilder WindSoilder marked this pull request as draft May 17, 2024 13:50
@WindSoilder
Copy link
Collaborator Author

Make as draft because the decision is not finally made

@NotTheDr01ds
Copy link
Contributor

NotTheDr01ds commented Jun 6, 2024

Coming to this based on #13077 - As a user, I think the existing semantic is much more logical. I know that various languages and libraries are split on this:

  • In PHP (not necessarily a language I want to mimic, but ...) range(1,3) results in 1, 2, and 3.
  • In JavaScript, the d3 library does the same, but Lodash results in 1 and 2 only.
  • Python range(1, 3) results in 1 and 2 only

Personally, I see 1..3 as more logically 1..=3, with 1..<3 being available if you need the alternative semantic.

On the flip side, I fully understand that slicing operations should (and must) be based on the rule "0 is the position before the first element, count up from there", which means that length + 1 is the position after the last character. E.g.:

> [ 1 2 3 4 ] | range 0..4
[ 1 2 3 ]

... and the same, as you mentioned, for str substring.

@WindSoilder
Copy link
Collaborator Author

WindSoilder commented Jun 10, 2024

@NotTheDr01ds Thanks for your input! Happy to see there are some languages return inclusive pair on something like 1..3(PHP in your example)
I'm ok with keeping current semantic.

So I will close the pr

@stevenxxiu
Copy link
Contributor

I support this PR. Rust and Python are very popular languages, with Nushell being written in Rust too. It'll be more familiar for people coming from those, especially when manipulating strings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes pr:commands This PR changes our commands in some way pr:language This PR makes some language changes pr:release-note-mention Addition/Improvement to be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ranges and str substring are inconsistent (off by 1)
3 participants