[ysh] Implement 1..<5 and 1..=5 open/closed range syntax #2102
Conversation
Also add an error message for the now removed 1..5 syntax.
PossiblyAShrub
force-pushed
the
add-closed-range-iter
branch
from
41013fc
to
351de42
Compare
|
This is ready for a review. |
There was a problem hiding this comment.
This is excellent, thanks for tracking it all down!
I was thinking if we want to pretty print the ranges separately, like
$ = 1 ..= 3
(Range) 1 ..= 3
But I think the way it is may be more transparent in a sense - people can see the equivalence between the two range syntaxes. It is printed in one canonical form
So it looks good other than some tiny doc tweaks
doc/error-catalog.md
Outdated
| ``` | ||
|
|
||
| There are two types of [range syntax](ref/chap-expr-lang#range). The `..<` | ||
| syntax is for half-open ranges and `..=` is for closed ranges: |
There was a problem hiding this comment.
I would s/syntax/operator to be precise
"The ..< operator is for half-open ranges"
doc/ref/chap-expr-lang.md
Outdated
| echo $i | ||
| } | ||
| => 0 | ||
| => 1 | ||
| => 2 | ||
|
|
||
| As with slices, the last number isn't included. To iterate from 1 to n, you | ||
| can use this idiom: | ||
| The `..<` syntax is for half-open ranges. `..=` is for closed ranges: |
There was a problem hiding this comment.
I would move the operator before it's example:
A range is ... .
The
..<operator is for half-open ranges:
[example]
The
..=operator is for closed ranges:
[example]
I also like writing it with spaces everywhere, like 0 ..= 3 instead of 0..=3, although that reminds me we need a formatter
ysh/expr_eval.py
Outdated
| @@ -1260,6 +1260,10 @@ def _EvalExpr(self, node): | |||
| i2 = _ConvertToInt(self._EvalExpr(node.upper), | |||
| 'Range end should be Int', node.op) | |||
|
|
|||
| # i2 is a big int so we don't have to worry about overflow | |||
There was a problem hiding this comment.
yeah it should be a BigInt, but isn't yet.
The first step may be to raise a hard error for overflow, which is OK
Also unfortunately the ranges are still truncated in the lines below, I think we should fix that, though not sure if it should come before or after the BigInt changes mentioned
There was a problem hiding this comment.
Okay, I've removed the comment and left the TODO below it.
Where are we at with a BigInt implementation? I remember there were a few Zulip threads a while back.
|
This is ready for another review. |
BREAKING: The
a..bsyntax has now been removed:Depends on #2101 to prevent merge conflicts (assuming #2101 will be merged before this PR.)
TODO: