EdgeVerve / feel

Expression Language for creating and executing business rules in decision table based on DMN 1.1 specification for conformance level 3
MIT License
93 stars 48 forks source link

1,2,3,4 (list of any type) parsed as error #7

Closed gustavomick closed 7 years ago

gustavomick commented 7 years ago

hi, i was testing few examples and looks like this case is not working. is it valid?

pragyandas commented 7 years ago

list is defined as [1,2,3,4]

gustavomick commented 7 years ago

@pragyandas thanks for your response, ive seen other engines and examples are without square brackets, also 9.2.6 (or 9.2.13) doesnt includes intervals grammar as 9.2.8. could you have a sec could yo recheck? thanks.

ref: 9.2.6 simple expressions = simple expression , { "," , simple expression } ;

pragyandas commented 7 years ago

@gustavomick I cannot find the semantics of simple expressions in the DMN 1.1 specs. Can u please direct me to some resource which explains the usage of simple expressions.

gustavomick commented 7 years ago

Not sure what you asking for, is this useful?

image image

https://docs.camunda.org/manual/7.6/reference/dmn11/feel/language-elements/

gustavomick commented 7 years ago

considering those examples i reframe that "1 + 2 / = A" is not a valid expression for an input entry, since is not boolean expression right side , nor an arithmetic expression nor a simple value. hope it helps. thanks.

pragyandas commented 7 years ago

@gustavomick Thanks for directing me to the right resource. I will add simple expressions and make some tweaks to the grammar to invalidate expressions like "1 + 2 / = A"

gustavomick commented 7 years ago

y np glad to help. good weekend

gustavomick commented 7 years ago

@pragyandas wondering if there is any estimated resolution time on this one? thanks!

pragyandas commented 7 years ago

I am working on this one right now. I should be able to update the status soon.

gustavomick commented 7 years ago

hi @pragyandas i was considering about using this library, but since we have no updates since last month i'm not sure about continuity. is this still alive? thanks

raghav135 commented 7 years ago

It is very much active, here is what we are working on:

  1. DRG/ DRD design and initial implementation
  2. Correcting decision table expression
  3. Deeper integration with oecloud model relations (Example: writing expression parent.parent.name for example will fetch parent from parentId twice)

By the way we are also planning to open source bpmn implementation in node.js soon.

gustavomick commented 7 years ago

Seems a lot going on there, any chance to have this small fix separated from this big push? thanks

pragyandas commented 7 years ago

@gustavomick I tried adding simple expressions but the grammar for it is parallel to grammar for expression. As the parser used in FEEL is a parser with single allowedStartRules, it is not possible to implement this with the current configuration. However, I will try to generate a parser with multiple allowedStartRules and check if it works. Will update you soon on this.

pragyandas commented 7 years ago

@gustavomick I have added simple expressions to the grammar. It has been made a different grammar entry point.

As the usage of simple expressions is very specific to input and output entries in a decision table, a parallel grammar implementation is desired.

The following code snippet illustrates the usage of the same :

const FEEL = require('./dist/feel');

const context = '{a : 1, b:2, c:3, d:4}';
const parsedContext = FEEL.parse(context); // parsed with default entry point

const text = 'a,b,c,d'
const parsedText = FEEL.parse(text,{startRule : "SimpleExpressions"}); // parsed with "SimpleExpressions" entry point

parsedContext.build().then(ctx => {
  return parsedText.build(ctx);
}).then((result) => {
    console.log(result);
}).catch(err => {
    console.error(err);
});
gustavomick commented 7 years ago

Hey, thanks for your feedback, could you expand on what you mean "specific to input and out entries", i'm confused .. is not "input / outputs exps" the main purpose of FEEL existence?

From what i see from dmn standard (ref above) is not a separated grammar, is core to grammar itself , rule number 1) "expression = simple expression"

again thanks for your thoughts on this, may be im misunderstanding something here.

pragyandas commented 7 years ago

@gustavomick I think you are referring to S-FEEL (the subset of FEEL). In FEEL expression and simple expressions are parallel to each other (DMN 1.1 clause 10). In the previous comment, I talked about input and output entries of a decision table and not I/O expressions.

As input/output entries are not implemented in the current decision table implementation, I can't include an example at this point.

gustavomick commented 7 years ago

thanks, i haven't found the reference you are mentioning regarding that they are parallel (could be a spec ambiguity) so if you can provide me with page / sections would be super useful,

in mean time few comments that to me worth debating when you have a moment to read:

1 - I think there is a kind of contradiction here. if we say s-feel is a subset, we are implying that is "part of" so included on Feel, and not an alternative grammar. 2 - Conformance Level 3 (FEEL) must be fully compliant with Level 2 (S-FEEL), is not possible to conform L 3 without L 2, so s-feel clauses are a must, from conformance pov. 3- Considering dmn spec FEEL grammar rule is also saying (ref picture) simple expression = arithmetic expression | simple value => textual expression = simple expression => expression = simple expression . I'm not fully tracing this grammar tree but only this branch makes simple exp a requirement. 4- and they there is a usability factor, we can say not(1,2,3) is valid, but 1,2,3 is not (among other cases) . i can not "sell" that, to me language primarily have to make sense from user pov.

thanks again

image

pragyandas commented 7 years ago

@gustavomick As it can be noted from the spec, S-FEEL is primarily for the purpose of implementation in decision table only. But there are other Boxed Expressions and decision table is one of them. According to the FEEL grammar specs, expression is not directly related to simple expressions in any way, neither are simple positive unary tests and unary tests. Considering the complexity of the grammar specification it is necessary to have multiple start rules for the grammar implementation. Simply put, the start rules create some sort of grouping for the parsing of the grammar and can be used for parsing different aspects of the language. Moreover, as FEEL is not a context-free grammar, there is a need for multiple entry points to provide some context for usability.

simple expression = arithmetic expression | simple value => textual expression = simple expression => expression = simple expression

Regarding this, tracing would work as textual expression => arithmetic expression but not vice versa (arithmetic expression | simple value => textual expression won't work)

I would also like to quote this article, https://methodandstyle.com/how-dmn-is-different/. As mentioned, no tool has ever been able to implement full FEEL specification because of the ambiguity and its dissimilarity with traditional rule engine implementations. We are trying to accommodate the entire FEEL implementation in this project.

Also, if you only need S-FEEL, simple expressions start rule would enable you to achieve it.

It should be noted that simple expressions is not used to parse rule list in a decision table but rather simple positive unary tests and unary tests are.

gustavomick commented 7 years ago

Ok i have other comments but I think at this point we can agree to disagree. :) I mean I wanted you to recheck this before were too late and need it a major refactor but if you think is the right path is fine by me.

Let me know if you have any second thoughts about this. Thanks for your feedback and have a good day.

pragyandas commented 7 years ago

@gustavomick Well, sometimes it is very difficult to find sense out the specification in a very straight forward way as it is kinda cryptic at places and there are no clear demarcations for these kinds of things. I figured it out the hard way but PEG.js saved me with the option --allowed-start-rules. But it is a very common practice for languages to have multiple start rules to allow complexity. I will still keep an eye open for the issues that might come in while analyzing this. You too have a great day. Thanks. :)