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 to omit indices in index range expansions #6574

Merged

Conversation

krobelus
Copy link
Member

@krobelus krobelus commented Feb 7, 2020

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/-1 in 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:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.md

@krobelus krobelus force-pushed the index-range-expansion-implied-limits branch from 6ff7829 to fea9db3 Compare February 7, 2020 16:03
Copy link
Member

@ridiculousfish ridiculousfish left a comment

Choose a reason for hiding this comment

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

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.

src/expand.cpp Show resolved Hide resolved
src/expand.cpp Outdated Show resolved Hide resolved
tests/test8.in Outdated
# missing start, cannot use implied range
echo $test[1..2 ..]
echo $test[..1 ..2]

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@krobelus krobelus force-pushed the index-range-expansion-implied-limits branch from fea9db3 to bba11c3 Compare February 9, 2020 10:40
@faho
Copy link
Member

faho commented Feb 9, 2020

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!

@krobelus krobelus force-pushed the index-range-expansion-implied-limits branch from bba11c3 to e5b395d Compare February 10, 2020 17:30
@krobelus
Copy link
Member Author

This prints the entire list, which seems logical however you slice it.

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.
@krobelus krobelus force-pushed the index-range-expansion-implied-limits branch from e5b395d to 5f02277 Compare February 10, 2020 17:37
@krobelus krobelus merged commit be06f84 into fish-shell:master Feb 10, 2020
@krobelus krobelus added this to the fish 3.2.0 milestone Feb 10, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants