feat: allow negative index/end on std.slice #1093
Conversation
|
@sbarzowski Will this "just work" on go-jsonnet? I think I was originally opinionated about not supporting this as I wanted the language to be simple... But in retrospect it seems more confusing to support python slicing only "partially" so this seems a good idea. |
Yes. I think we just desugar to BTW this change definitely requires an update to documentation. |
Upstream issue: google/jsonnet#1093
|
It seems like this was almost ready to go back in June, with only a request for documentation updates as a possible blocker. @Duologic would you like to contribute some doc changes for this, or should I work on it? @sparkprime |
|
I just scanned the documentation and didn't find any mention that negative slices are not supported, am I missing something? Do you want an example added maybe? |
|
Hmm... it's a good point, the tutorial says "Array slices like IMHO, yes, it would be good to add an example of a negative index in the |
There was a problem hiding this comment.
If you're still willing to work on this PR, could you fix the way inval.index is calculated, so that the behaviour of ("012345")[-7:] matches Python. And add string and array test cases for that in test_suite/slice.sugar.jsonnet. (See my inline code comment)
I realise it's been a while since this PR was first opened, so your motivation or priorities may have changed. If so, please let me know and I'm happy to do the remaining parts and get this merged.
…fmt test golden Slice is documented to behave similarly to Python. Negative indexes in Python slices are clamped so slice indexes never produce out-of-range runtime errors. Update the 'slice' sugar to do the same. Note that only the starting index needs to be clamped in this way; the ending index can be out-of-range doesn't cause an error, as the loop exit condition compares the current index against the ending index.
johnbartholomew
force-pushed
the
duologic/slice_negative
branch
from
e9a2fb3
to
8c53f3c
Compare
|
Rebased and merged. :-) |
Upstream issue: google/jsonnet#1093
I found myself implementing this elsewhere and figured it'd be easy to support in stdlib.
I ran
make test, it succeeded for my changes but had unrelated failures, we might want to fix that in a separate PR.