defs #6

rzukow · 2016-09-20T15:28:03Z

No description provided.

bartoszwalacik · 2016-09-20T15:51:19Z

src/main/java/pl/allegro/tech/opel/OpelParser.java

@@ -134,6 +134,18 @@ Rule MultiplyExpression() {
        );
    }

+    Rule Program() {
+        return Sequence(push(nodeFactory.emptyDeclarationsList()), Declarations(), Expression(), push(nodeFactory.program(pop(1), pop())));


looks confusing

hopefully fixed

bartoszwalacik · 2016-09-20T15:52:41Z

src/test/groovy/pl/allegro/tech/opel/OpelEngineDefinitionsIntegrationSpec.groovy

+import static pl.allegro.tech.opel.OpelEngineBuilder.create
+import static pl.allegro.tech.opel.TestUtil.functions
+
+class OpelEngineDefinitionsIntegrationSpec extends Specification {


pawel-piecyk · 2016-09-20T20:17:15Z

src/main/java/pl/allegro/tech/opel/OpelParser.java

@@ -19,7 +19,7 @@
    }

    Rule ParsingUnit() {
-        return Sequence(WhiteSpace(), Expression(), EOI);
+        return Sequence(WhiteSpace(), Program(), EOI);


First step towards something more than simple expression language :) +1

pawel-piecyk · 2016-09-20T20:22:48Z

src/test/groovy/pl/allegro/tech/opel/OpelEngineDefinitionsIntegrationSpec.groovy

+
+class OpelEngineDefinitionsIntegrationSpec extends Specification {
+
+    def "should parse and evaluate expression with definitions (#input)"() {


Add @Unroll here and below or remove #input.

tfij · 2016-09-21T01:10:45Z

README.md

@@ -66,6 +66,7 @@ opel supports:
 - simple list element access (i.e. `list[index]`)
 - object method calls (i.e. `'Hello, World!'.length()`)
 - if expressions (i.e. `if (2 > 3) 'a' else 'b'`)
+- defining local final variables (i.e. `def x = 2+2*2; x * x`)


"local final variables" sounds like value, what about val instead of def?

def sound good for me (because of Groovy :)

tfij · 2016-09-21T01:12:56Z

README.md

@@ -66,6 +66,7 @@ opel supports:
 - simple list element access (i.e. `list[index]`)
 - object method calls (i.e. `'Hello, World!'.length()`)
 - if expressions (i.e. `if (2 > 3) 'a' else 'b'`)
+- defining local final variables (i.e. `def x = 2+2*2; x * x`)


this change is in conflict with "What opel can't do?" section of this readme :)

That's my biggest gripe, do we change the vision?

we should, each non-braking feature - MINOR ++
http://semver.org/

tfij · 2016-09-21T01:16:52Z

src/main/java/pl/allegro/tech/opel/OpelParser.java

+    }
+
+    Rule Declaration() {
+        return Sequence("def ", Identifier(), "= ", Expression(), "; ", push(nodeFactory.declarationsList(pop(2), pop(1), pop())));


what about new line instead semicolon?

tfij · 2016-09-21T01:21:53Z

src/main/java/pl/allegro/tech/opel/DeclarationExpressionNode.java

+    }
+
+    @Override
+    public CompletableFuture<?> getValue(EvalContext context) {


in grammar you add Declarations beside Expression so DeclarationExpressionNode should not implements ExpressionNode (Declaration is not an Expression) and problem with this UnsupportedOperationException disappear :)

The problem is that parser needs a single node type to work with, and not all nodes will have values. I will try to improve it.

tfij · 2016-09-21T01:41:20Z

src/test/groovy/pl/allegro/tech/opel/OpelEngineDefinitionsIntegrationSpec.groovy

+        where:
+        input                                                     || expResult
+        "def one=1;one"                                           || 1
+        "def two=1+1;two + 1"                                     || 3


expResult is 1, 3, 4, 5. Where is 2 :)

tfij · 2016-09-21T01:42:16Z

src/test/groovy/pl/allegro/tech/opel/OpelEngineDefinitionsIntegrationSpec.groovy

+        "def two=1+1; def three = two +1; three + 1"              || 4
+        "def condition=1==1; if(condition) 5 else 6"              || 5
+
+    }


add empty line

tfij · 2016-09-21T01:45:15Z

src/test/groovy/pl/allegro/tech/opel/OpelEngineDefinitionsIntegrationSpec.groovy

+        "def condition=1==1; if(condition) 5 else 6"              || 5
+
+    }
+    def "should end with error when circular definitions are found in #input"() {


in this test you check circular definitions. Where is test of declaration order:

def a = b; def b = 1; a + b

is it ok?

Undefined variable b (in line 1). It's the same case as circular dependency

tfij · 2016-09-21T01:46:47Z

src/main/java/pl/allegro/tech/opel/ProgramExpressionNode.java

+import java.util.*;
+import java.util.concurrent.CompletableFuture;
+
+public class ProgramExpressionNode implements ExpressionNode {


the question is whether the program is an expression or not

True, I think I will intruduce OpelNode instead

tfij · 2016-09-21T01:51:28Z

src/test/groovy/pl/allegro/tech/opel/OpelEngineDefinitionsIntegrationSpec.groovy

+        "def condition=1==1; if(condition) 5 else 6"              || 5
+
+    }
+    def "should end with error when circular definitions are found in #input"() {


and add specification of conflict case when some value is defined in context and manually by def keyword. It is an error or which declaration has higher priority?

I was thinking that def's override registered variables

tfij

a few things to improve and discuss

Defs -> vals

Refactor updatedContext method in ProgramNode class

defs

d2548c2

bartoszwalacik reviewed Sep 20, 2016

View reviewed changes

CR fix

e45df43

pawel-piecyk reviewed Sep 20, 2016

View reviewed changes

tfij reviewed Sep 21, 2016

View reviewed changes

Tomasz Fijalkowski and others added 5 commits September 22, 2016 18:27

Some tests and rename ExpressionNode to OpelNode

8361af9

val keyworld nstead of def

2322d34

Merge pull request #1 from tfij/defs

4c496bb

Defs -> vals

Refactor updatedContext method in ProgramNode class

de45179

Merge pull request #2 from tfij/defs

152ec48

Refactor updatedContext method in ProgramNode class

rzukow changed the base branch from master to defining_values September 23, 2016 12:13

rzukow merged commit 6077b06 into allegro:defining_values Sep 23, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

defs #6

defs #6

rzukow commented Sep 20, 2016

bartoszwalacik Sep 20, 2016

rzukow Sep 20, 2016

bartoszwalacik Sep 20, 2016

pawel-piecyk Sep 20, 2016

pawel-piecyk Sep 20, 2016

tfij Sep 21, 2016

bartoszwalacik Sep 21, 2016

tfij Sep 21, 2016

tfij Sep 21, 2016

bartoszwalacik Sep 21, 2016

tfij Sep 21, 2016

tfij Sep 21, 2016

rzukow Sep 21, 2016

tfij Sep 21, 2016

tfij Sep 21, 2016

tfij Sep 21, 2016

rzukow Sep 21, 2016 •

edited

Loading

tfij Sep 21, 2016

rzukow Sep 21, 2016

tfij Sep 21, 2016

rzukow Sep 21, 2016

tfij left a comment


		class OpelEngineDefinitionsIntegrationSpec extends Specification {

		def "should parse and evaluate expression with definitions (#input)"() {

defs #6

defs #6

Conversation

rzukow commented Sep 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzukow Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfij left a comment

Choose a reason for hiding this comment

rzukow Sep 21, 2016 •

edited

Loading