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

Enable comparison feature like python #664

Closed
wants to merge 3 commits into from

Conversation

ckganesan
Copy link
Contributor

This pull request adds support for a comparison feature similar to Python.

ok not in [false] && 10 == 10 && 0.5 < 2 == ok not in [false]
1 > 2 < 3
30 > 19 in 18..31 != true
5 == x > 4

@antonmedv
Copy link
Member

Expr already has chained comp operation (#581)

1 < x < 9

One issue we encountered with implementing Python style ComparisonNode is backward comp.

In Expr next expressions parsed as left:

1 < 2 == true
(1 < 2) == true
(true) == true

So implementing == was not feasible. And we implemented only < > <= >=.

I think those comparison should cover much of the requirements.

Also I think allowing to use == leads to whore DX. For example, 30 > 19 in 18..31 != true how to read this?

30 > 19 in 18..31 != true
(30 > 19) in 18..31 != true
30 > (19 in 18..31) != true

@bizywizy may remember more.

@bizywizy
Copy link
Contributor

bizywizy commented Jun 3, 2024

Yes, I think it's too late to change this behaviour

42 == 42 == true // expr: true; python: false
13 < 42 == true // expr: true; python: false
13 > 2 == true // expr: true; python: false

@ckganesan
Copy link
Contributor Author

ckganesan commented Jun 3, 2024

Yes, I think it's too late to change this behaviour

42 == 42 == true // expr: true; python: false
13 < 42 == true // expr: true; python: false
13 > 2 == true // expr: true; python: false

This change won`t affect expr functionality but it will use the functionality like python

42 == 42 == true
CompareNode{
        Left: IntegerNode{
                Value: 42,
        },
        Operators: []string{
                "==",
                "==",
        },
        Comparators: []ast.Node{
                IntegerNode{
                        Value: 42,
                },
                BoolNode{
                        Value: true,
                },
        },
}
0  OpPush  <0>  42
1  OpPush  <0>  42
2  OpEqualInt
3  OpTrue
4  OpEqual

true
13 < 42 == true
CompareNode{
        Left: IntegerNode{
                Value: 13,
        },
        Operators: []string{
                "<",
                "==",
        },
        Comparators: []ast.Node{
                IntegerNode{
                        Value: 42,
                },
                BoolNode{
                        Value: true,
                },
        },
}
0  OpPush  <0>  13
1  OpPush  <1>  42
2  OpLess
3  OpTrue
4  OpEqual

true
13 > 2 == true
CompareNode{
        Left: IntegerNode{
                Value: 13,
        },
        Operators: []string{
                ">",
                "==",
        },
        Comparators: []ast.Node{
                IntegerNode{
                        Value: 2,
                },
                BoolNode{
                        Value: true,
                },
        },
}
0  OpPush  <0>  13
1  OpPush  <1>  2
2  OpMore
3  OpTrue
4  OpEqual

true

@ckganesan
Copy link
Contributor Author

Expr already has chained comp operation (#581)

1 < x < 9

One issue we encountered with implementing Python style ComparisonNode is backward comp.

In Expr next expressions parsed as left:

1 < 2 == true
(1 < 2) == true
(true) == true

So implementing == was not feasible. And we implemented only < > <= >=.

I think those comparison should cover much of the requirements.

Also I think allowing to use == leads to whore DX. For example, 30 > 19 in 18..31 != true how to read this?

30 > 19 in 18..31 != true
(30 > 19) in 18..31 != true
30 > (19 in 18..31) != true

@bizywizy may remember more.

The == and != operators function based on the right-hand expression. If the right-hand expression is a boolean, the left-hand expression will be evaluated as a boolean. Ex : 30 > 19 in 18..31 != true

30 > 19 && (18 <= 19 <= 31) != true
CompareNode{
        Left: IntegerNode{
                Value: 30,
        },
        Operators: []string{
                ">",
                "&&",
                "!=",
        },
        Comparators: []ast.Node{
                IntegerNode{
                        Value: 19,
                },
                CompareNode{
                        Left: IntegerNode{
                                Value: 18,
                        },
                        Operators: []string{
                                "<=",
                                "<=",
                        },
                        Comparators: []ast.Node{
                                IntegerNode{
                                        Value: 19,
                                },
                                IntegerNode{
                                        Value: 31,
                                },
                        },
                },
                BoolNode{
                        Value: true,
                },
        },
}
0   OpPush  <0>  30
1   OpPush  <1>  19
2   OpMore
3   OpJumpIfFalse  <9>  (13)
4   OpPop
5   OpPush  <2>  18
6   OpPush  <1>  19
7   OpLessOrEqual
8   OpJumpIfFalse  <4>  (13)
9   OpPop
10  OpPush  <1>  19
11  OpPush  <3>  31
12  OpLessOrEqual
13  OpTrue
14  OpEqual
15  OpNot

false

@antonmedv
Copy link
Member

Then I don't understand what this PR tries to implement. What's different?

@ckganesan
Copy link
Contributor Author

Then I don't understand what this PR tries to implement. What's different?

This PR supports all comparison operators(>, >=, <, <=, in, !=, ==, in, matches, contains, startsWith, endsWith) in chained comparison operations.

@antonmedv
Copy link
Member

But why?

@antonmedv antonmedv closed this Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants