You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by GitBox <gi...@apache.org> on 2021/04/10 13:47:34 UTC
[GitHub] [groovy] Exploder98 opened a new pull request #1553: Implement GROOVY-9649: left-open and full-open ranges
Exploder98 opened a new pull request #1553:
URL: https://github.com/apache/groovy/pull/1553
[JIRA Issue](https://issues.apache.org/jira/browse/GROOVY-9649)
This Pull request implements the above feature and adds test cases related to left- and full-open ranges.
We have also added documentation.
This change *should* be backwards-compatible, as old right-exclusive ranges work as before.
We have tried to preserve all old constructors and methods so that this change would be as backwards-compatible as possible.
A few things to note:
- ranges like `0<..<1` where to and from differ by one, the range is full exclusive and to and from share the same sign (positive numbers and zero are considered to have the same sign) are created as `EmptyRange`s.
This is because ranges like `0<..<-1` can still be used to index into lists.
- while testing, we noticed that the following equality test produces different results when the operands are switched:
```groovy
IntRange r1 = new IntRange(false, 1, 10) // "Inclusive-aware" range
IntRange r2 = new IntRange(1, 10) // Non-inclusive-aware range
r1.equals(r2) // Returns false
r2.equals(r1) // Returns true
```
we have not changed this behaviour.
While implementing the feature, we noticed some inconsistencies between different ranges, which we have not altered due to backwards compatibility:
- `NumberRange` and `IntRange` contain information about their inclusivity, but `ObjectRange` inclusivity is taken into account when the range is created
- in `IntRange`, the `getFrom` and `getTo` methods return the actual bounds of that range, whereas `NumberRange`'s corresponging methods return the bounds that the object was created with, even if the bounds are not actually included in the range
This PR was a joint effort by 4 people:
@Exploder98
@iiroki
@OttoVayrynen
@EricssonWilli
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] eric-milles commented on a change in pull request #1553: Implement GROOVY-9649: left-open and full-open ranges
Posted by GitBox <gi...@apache.org>.
eric-milles commented on a change in pull request #1553:
URL: https://github.com/apache/groovy/pull/1553#discussion_r611073139
##########
File path: src/main/java/org/codehaus/groovy/ast/expr/RangeExpression.java
##########
@@ -27,26 +27,42 @@
* <pre>for i in 0..10 {...}</pre>
*/
public class RangeExpression extends Expression {
-
private final Expression from;
private final Expression to;
- private final boolean inclusive;
+ private final boolean inclusive; // Kept to keep old code depending on this working
+ // GROOVY-9649
+ private final boolean exclusiveLeft;
+ private final boolean exclusiveRight;
+ // Kept until sure this can be removed
public RangeExpression(Expression from, Expression to, boolean inclusive) {
this.from = from;
this.to = to;
this.inclusive = inclusive;
+ this.exclusiveLeft = false;
+ this.exclusiveRight = !inclusive;
+ setType(ClassHelper.RANGE_TYPE);
Review comment:
Can this constructor delegate to the new constructor?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on pull request #1553: Implement GROOVY-9649: left-open and full-open ranges
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1553:
URL: https://github.com/apache/groovy/pull/1553#issuecomment-817701212
FYI, we will likely only put this in Groovy 4.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on pull request #1553: Implement GROOVY-9649: left-open and full-open ranges
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1553:
URL: https://github.com/apache/groovy/pull/1553#issuecomment-817700262
Merged - with a minor tweak for the conflict. Thanks! I suspect we will need to tweak a few other things, e.g. containsWithinBounds and perhaps time to relook at equals symmetry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] iiroki commented on a change in pull request #1553: Implement GROOVY-9649: left-open and full-open ranges
Posted by GitBox <gi...@apache.org>.
iiroki commented on a change in pull request #1553:
URL: https://github.com/apache/groovy/pull/1553#discussion_r611150168
##########
File path: src/main/java/org/codehaus/groovy/ast/expr/RangeExpression.java
##########
@@ -27,26 +27,42 @@
* <pre>for i in 0..10 {...}</pre>
*/
public class RangeExpression extends Expression {
-
private final Expression from;
private final Expression to;
- private final boolean inclusive;
+ private final boolean inclusive; // Kept to keep old code depending on this working
+ // GROOVY-9649
+ private final boolean exclusiveLeft;
+ private final boolean exclusiveRight;
+ // Kept until sure this can be removed
public RangeExpression(Expression from, Expression to, boolean inclusive) {
this.from = from;
this.to = to;
this.inclusive = inclusive;
+ this.exclusiveLeft = false;
+ this.exclusiveRight = !inclusive;
+ setType(ClassHelper.RANGE_TYPE);
Review comment:
Agreed, I'll make this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] iiroki commented on a change in pull request #1553: Implement GROOVY-9649: left-open and full-open ranges
Posted by GitBox <gi...@apache.org>.
iiroki commented on a change in pull request #1553:
URL: https://github.com/apache/groovy/pull/1553#discussion_r611151249
##########
File path: src/main/java/org/codehaus/groovy/ast/expr/RangeExpression.java
##########
@@ -64,10 +80,20 @@ public boolean isInclusive() {
return inclusive;
}
+ public boolean isExclusiveLeft() {
+ return exclusiveLeft;
+ }
+
+ public boolean isExclusiveRight() {
+ return exclusiveRight;
+ }
+
@Override
Review comment:
I agree that it would be clearer to use "open" keyword instead of "exclusive". `isOpenLeft` and `isOpenRight` are more similiar to each other than `isLeftOpen` and `isRightOpen` so it think it would be better to go with that way. IDE autocompletion will also benefit from this, e.g. writing `isOpen...` will pop up both.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] iiroki commented on a change in pull request #1553: Implement GROOVY-9649: left-open and full-open ranges
Posted by GitBox <gi...@apache.org>.
iiroki commented on a change in pull request #1553:
URL: https://github.com/apache/groovy/pull/1553#discussion_r611151249
##########
File path: src/main/java/org/codehaus/groovy/ast/expr/RangeExpression.java
##########
@@ -64,10 +80,20 @@ public boolean isInclusive() {
return inclusive;
}
+ public boolean isExclusiveLeft() {
+ return exclusiveLeft;
+ }
+
+ public boolean isExclusiveRight() {
+ return exclusiveRight;
+ }
+
@Override
Review comment:
I agree that it would be clearer to use "open" keyword instead of "exclusive". `isOpenLeft` and `isOpenRight` are more similiar to each other than `isLeftOpen` and `isRightOpen` so it think it would be better to go with that way. IDE autocompletion will also benefit from this, e.g. writing `isOpen...` will pop up both. I'll make the change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] eric-milles commented on a change in pull request #1553: Implement GROOVY-9649: left-open and full-open ranges
Posted by GitBox <gi...@apache.org>.
eric-milles commented on a change in pull request #1553:
URL: https://github.com/apache/groovy/pull/1553#discussion_r611073338
##########
File path: src/main/java/org/codehaus/groovy/ast/expr/RangeExpression.java
##########
@@ -64,10 +80,20 @@ public boolean isInclusive() {
return inclusive;
}
+ public boolean isExclusiveLeft() {
+ return exclusiveLeft;
+ }
+
+ public boolean isExclusiveRight() {
+ return exclusiveRight;
+ }
+
@Override
Review comment:
Would "isLeftExclusive" or "isLeftOpen" be better than "isExclusiveLeft"? And same for right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] iiroki commented on a change in pull request #1553: Implement GROOVY-9649: left-open and full-open ranges
Posted by GitBox <gi...@apache.org>.
iiroki commented on a change in pull request #1553:
URL: https://github.com/apache/groovy/pull/1553#discussion_r611153196
##########
File path: src/main/java/org/codehaus/groovy/ast/expr/RangeExpression.java
##########
@@ -64,10 +80,20 @@ public boolean isInclusive() {
return inclusive;
}
+ public boolean isExclusiveLeft() {
+ return exclusiveLeft;
+ }
+
+ public boolean isExclusiveRight() {
+ return exclusiveRight;
+ }
+
@Override
Review comment:
On second thought, "exclusive" keyword is widely used elsewhere, so keeping the names as they are now would keep things more consistent.
##########
File path: src/main/java/org/codehaus/groovy/ast/expr/RangeExpression.java
##########
@@ -64,10 +80,20 @@ public boolean isInclusive() {
return inclusive;
}
+ public boolean isExclusiveLeft() {
+ return exclusiveLeft;
+ }
+
+ public boolean isExclusiveRight() {
+ return exclusiveRight;
+ }
+
@Override
Review comment:
I agree that it would be clearer to use "open" keyword instead of "exclusive". `isOpenLeft` and `isOpenRight` are more similiar to each other than `isLeftOpen` and `isRightOpen` so it think it would be better to go with that way. IDE autocompletion will also benefit from this, e.g. writing `isOpen...` will pop up both.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] iiroki commented on a change in pull request #1553: Implement GROOVY-9649: left-open and full-open ranges
Posted by GitBox <gi...@apache.org>.
iiroki commented on a change in pull request #1553:
URL: https://github.com/apache/groovy/pull/1553#discussion_r611150145
##########
File path: src/main/java/org/codehaus/groovy/ast/expr/RangeExpression.java
##########
@@ -27,26 +27,42 @@
* <pre>for i in 0..10 {...}</pre>
*/
public class RangeExpression extends Expression {
-
private final Expression from;
private final Expression to;
- private final boolean inclusive;
+ private final boolean inclusive; // Kept to keep old code depending on this working
Review comment:
Agreed, I'll implement this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] EricssonWilli commented on pull request #1553: Implement GROOVY-9649: left-open and full-open ranges
Posted by GitBox <gi...@apache.org>.
EricssonWilli commented on pull request #1553:
URL: https://github.com/apache/groovy/pull/1553#issuecomment-817669564
There seems to be a conflict, should we resolve that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] eric-milles commented on a change in pull request #1553: Implement GROOVY-9649: left-open and full-open ranges
Posted by GitBox <gi...@apache.org>.
eric-milles commented on a change in pull request #1553:
URL: https://github.com/apache/groovy/pull/1553#discussion_r611073690
##########
File path: src/main/java/org/codehaus/groovy/ast/expr/RangeExpression.java
##########
@@ -27,26 +27,42 @@
* <pre>for i in 0..10 {...}</pre>
*/
public class RangeExpression extends Expression {
Review comment:
Is there any hope that code like AST transforms that depend on "from" and "to" and "inclusive" will be able to handle a left-open or full-open range literal? Is it considered a breaking change to use "<" on the left?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] EricssonWilli edited a comment on pull request #1553: Implement GROOVY-9649: left-open and full-open ranges
Posted by GitBox <gi...@apache.org>.
EricssonWilli edited a comment on pull request #1553:
URL: https://github.com/apache/groovy/pull/1553#issuecomment-817669564
There seems to be a conflict, should we resolve that?
Edit:
This seems to be merged, thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] eric-milles commented on a change in pull request #1553: Implement GROOVY-9649: left-open and full-open ranges
Posted by GitBox <gi...@apache.org>.
eric-milles commented on a change in pull request #1553:
URL: https://github.com/apache/groovy/pull/1553#discussion_r611073061
##########
File path: src/main/java/org/codehaus/groovy/ast/expr/RangeExpression.java
##########
@@ -27,26 +27,42 @@
* <pre>for i in 0..10 {...}</pre>
*/
public class RangeExpression extends Expression {
-
private final Expression from;
private final Expression to;
- private final boolean inclusive;
+ private final boolean inclusive; // Kept to keep old code depending on this working
Review comment:
Can this field be dropped and `isInclusive()` is implemented in terms of "right inclusivity"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert closed pull request #1553: Implement GROOVY-9649: left-open and full-open ranges
Posted by GitBox <gi...@apache.org>.
paulk-asert closed pull request #1553:
URL: https://github.com/apache/groovy/pull/1553
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] eric-milles commented on a change in pull request #1553: Implement GROOVY-9649: left-open and full-open ranges
Posted by GitBox <gi...@apache.org>.
eric-milles commented on a change in pull request #1553:
URL: https://github.com/apache/groovy/pull/1553#discussion_r611073061
##########
File path: src/main/java/org/codehaus/groovy/ast/expr/RangeExpression.java
##########
@@ -27,26 +27,42 @@
* <pre>for i in 0..10 {...}</pre>
*/
public class RangeExpression extends Expression {
-
private final Expression from;
private final Expression to;
- private final boolean inclusive;
+ private final boolean inclusive; // Kept to keep old code depending on this working
Review comment:
Can this field be dropped and `isInclusive()` is implemented in terms of "right exclusivity"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org