Review Range and Interval, resolve infinite loop issue #3408
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jdunkerley
reviewed
Apr 19, 2022
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Interval.enso
Outdated
Show resolved
Hide resolved
jdunkerley
reviewed
Apr 19, 2022
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Interval.enso
Outdated
Show resolved
Hide resolved
jdunkerley
reviewed
Apr 19, 2022
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Number/Extensions.enso
Outdated
Show resolved
Hide resolved
jdunkerley
requested changes
Apr 19, 2022
hubertp
reviewed
Apr 20, 2022
| @@ -27,7 +64,9 @@ type Range | |||
|
|
|||
| 0.up_to 100 . is_empty | |||
| is_empty : Boolean | |||
| is_empty = this.end <= this.start | |||
| is_empty = if this.step > 0 then this.end <= this.start else | |||
There was a problem hiding this comment.
Interval.inclusive 0 0 means this will throw an error?
There was a problem hiding this comment.
Interval is not related to Range
...ntime/src/main/java/org/enso/interpreter/node/expression/builtin/number/decimal/ModNode.java
Show resolved
Hide resolved
|
|
||
| Test.specify "should report errors if trying to set step to 0" <| | ||
| 0.up_to 0 . with_step 0 . should_fail_with Illegal_State_Error | ||
| invalid_range = Range 0 0 0 |
There was a problem hiding this comment.
Maybe test Range 0 0 2 as well? Just to cover corner cases
There was a problem hiding this comment.
But this range is not invalid, it is just empty.
There was a problem hiding this comment.
A range is only invalid if its step is 0 - because the step should be a non-zero integer (so technically a range is also invalid if the step is decimal, but this is a typelevel check).
A range whose step exceeds end-start is still valid, but will be empty if start=end or contain just a single element [start].
radeusgd
force-pushed
the
wip/radeusgd/unify-range-interval-181435598
branch
from
1c7a199
to
6cc1faa
Compare
4e6
approved these changes
Apr 20, 2022
jdunkerley
reviewed
Apr 20, 2022
jdunkerley
approved these changes
Apr 20, 2022
TODO: shouldnt mod and % be separate operators like / and div are?
Co-authored-by: James Dunkerley <jdunkerley@users.noreply.github.com>
radeusgd
force-pushed
the
wip/radeusgd/unify-range-interval-181435598
branch
from
cfa9b49
to
80b771d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Description
Implements: https://www.pivotaltracker.com/story/show/181652841
Important Notes
Checklist
Please include the following checklist in your PR:
./run distand./run watch.