Allow to omit indices in index range expansions#6574
Allow to omit indices in index range expansions#6574krobelus merged 1 commit intofish-shell:masterfish-shell/fish-shell:masterfrom krobelus:index-range-expansion-implied-limitskrobelus/fish-shell:index-range-expansion-implied-limitsCopy head branch name to clipboard
Conversation
6ff7829 to
fea9db3
Compare
ridiculousfish
left a comment
There was a problem hiding this comment.
I like the language change, I added some comments on the implementation. I'd like @faho to approve too as he did a lot of work on ranges in the past.
One question is whether empty variables are treated the same as omitted indexes. In this implementation it looks like they are; if that's deliberate we should test and document it.
| # missing start, cannot use implied range | ||
| echo $test[1..2 ..] | ||
| echo $test[..1 ..2] | ||
|
|
There was a problem hiding this comment.
Please add a test that makes the behavior clear for an empty variable, e.g.:
echo $test[..$empty]
I do not have a strong opinion about what that should do, but it should be something we decide and test.
There was a problem hiding this comment.
Ah right, I added a test for it.
Turns out this expands to no indices because of cartesian product expansion. It is at least consistent with the same code with 1 on the left side.
fea9db3 to
bba11c3
Compare
|
So currently having an empty variable in a range is allowed, because it triggers the cartesian product (and hence removes the entire token). I.e. this doesn't error: set -l empty
set -l list 1 2 3
echo $list[1..$empty]
# even this is allowed:
echo $list[..$empty]Which means if we handled empty variables any differently here it would be a breaking change. I think the cartesian product here is consistent with it elsewhere and therefore the right thing, but then we've never been in love with it in general. However that would be a larger discussion, and I think it's one we could have later. So I would keep this as-is, except for one test I'd like to add: echo $list[.."$empty"]
echo $list["$empty"..]This prints the entire list, which seems logical however you slice it. Otherwise: LGTM! |
bba11c3 to
e5b395d
Compare
A+ pun right there 😂 😂 Let's ship it! |
Missing range limits in, say $PATH[..] default to the first/last element, just like Python/Go/Rust slices.
e5b395d to
5f02277
Compare
Description
So omitted range limits in, say
$PATH[..]default to the first/last element, just like Python/Go/Rust slices.This is something I have had expected to work several times (coming from Python). It essentially saves typing the
1/-1in a..range and I don't see any harm in defaulting to those values.However, it's not super well thought out, and a language change so I'm asking if there are any concerns.
TODOs: