You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kie.apache.org by "tkobayas (via GitHub)" <gi...@apache.org> on 2024/04/09 06:20:22 UTC
[PR] [incubator-kie-drools-5716] [new-parser] mismatched input 'if'/'do' (… [incubator-kie-drools]
tkobayas opened a new pull request, #5831:
URL: https://github.com/apache/incubator-kie-drools/pull/5831
…support named consequences)
**Issue**:
* https://github.com/apache/incubator-kie-drools/issues/5716
### Before PR in `drools-model-codegen`
```
[ERROR] Tests run: 2434, Failures: 167, Errors: 0, Skipped: 9
```
### After PR in `drools-model-codegen`
```
[ERROR] Tests run: 2434, Failures: 141, Errors: 0, Skipped: 9
```
Fixed all tests in `org.drools.model.codegen.execmodel.NamedConsequencesTest`
<details>
<summary>
How to replicate CI configuration locally?
</summary>
Build Chain tool does "simple" maven build(s), the builds are just Maven commands, but because the repositories relates and depends on each other and any change in API or class method could affect several of those repositories there is a need to use [build-chain tool](https://github.com/kiegroup/github-action-build-chain) to handle cross repository builds and be sure that we always use latest version of the code for each repository.
[build-chain tool](https://github.com/kiegroup/github-action-build-chain) is a build tool which can be used on command line locally or in Github Actions workflow(s), in case you need to change multiple repositories and send multiple dependent pull requests related with a change you can easily reproduce the same build by executing it on Github hosted environment or locally in your development environment. See [local execution](https://github.com/kiegroup/github-action-build-chain#local-execution) details to get more information about it.
</details>
<details>
<summary>
How to retest this PR or trigger a specific build:
</summary>
- for <b>pull request and downstream checks</b>
- Push a new commit to the PR. An empty commit would be enough.
- for a <b>full downstream build</b>
- for <b>github actions</b> job: add the label `run_fdb`
- for <b>Jenkins PR check only</b>
- If you are an ASF committer for KIE podling, login to Jenkins (https://ci-builds.apache.org/job/KIE/job/drools/), go to the specific PR job, and click on `Build Now` button.
</details>
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools-5716] [new-parser] mismatched input 'if'/'do' (… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on code in PR #5831:
URL: https://github.com/apache/incubator-kie-drools/pull/5831#discussion_r1558776854
##########
drools-drl/drools-drl-parser/src/main/antlr4/org/drools/drl/parser/antlr4/DRLParser.g4:
##########
@@ -488,9 +508,13 @@ lhsAccumulate : (DRL_ACCUMULATE|DRL_ACC) LPAREN lhsAndDef (COMMA|SEMI)
RPAREN (SEMI)?
;
-rhs : DRL_THEN consequence ;
+rhs : DRL_THEN defaultConsequence namedConsequence* ;
+
+defaultConsequence : ( RHS_STRING_LITERAL | RHS_CHUNK )* ;
-consequence : ( RHS_STRING_LITERAL | RHS_CHUNK )* ;
+// THEN LEFT_SQUARE ID RIGHT_SQUARE chunk
+namedConsequence : RHS_NAMED_CONSEQUENCE_THEN namedConsequenceBody ;
+namedConsequenceBody : ( RHS_STRING_LITERAL | RHS_CHUNK )* ;
Review Comment:
@yurloc Thank you. I applied the suggestion with a small name 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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools-5716] [new-parser] mismatched input 'if'/'do' (… [incubator-kie-drools]
Posted by "yurloc (via GitHub)" <gi...@apache.org>.
yurloc commented on code in PR #5831:
URL: https://github.com/apache/incubator-kie-drools/pull/5831#discussion_r1558383147
##########
drools-drl/drools-drl-parser/src/main/antlr4/org/drools/drl/parser/antlr4/DRLLexer.g4:
##########
@@ -172,6 +172,8 @@ RHS_STRING_LITERAL
| ('\'' ( DrlEscapeSequence | ~('\\'|'\'') )* '\'') { setText( normalizeString( getText() ) ); }
;
+RHS_NAMED_CONSEQUENCE_THEN : 'then' LBRACK IDENTIFIER RBRACK ;
Review Comment:
```suggestion
RHS_NAMED_CONSEQUENCE_THEN : DRL_THEN LBRACK IDENTIFIER RBRACK ;
```
##########
drools-drl/drools-drl-parser/src/main/java/org/drools/drl/parser/antlr4/DRLVisitorImpl.java:
##########
@@ -292,8 +294,17 @@ public RuleDescr visitRuledef(DRLParser.RuledefContext ctx) {
}
if (ctx.rhs() != null) {
+ // default consequence
ruleDescr.setConsequenceLocation(ctx.rhs().getStart().getLine(), ctx.rhs().getStart().getCharPositionInLine()); // location of "then"
- ruleDescr.setConsequence(trimThen(getTokenTextPreservingWhitespace(ctx.rhs(), tokenStream))); // RHS is just a text
+ ruleDescr.setConsequence(getTokenTextPreservingWhitespace(ctx.rhs().defaultConsequence(), tokenStream)); // RHS is just a text
+
+ // named consequences
+ ctx.rhs().namedConsequence()
+ .forEach(namedConsequnceCtx -> {
+ String name = extractNamedConsequenceName(namedConsequnceCtx.RHS_NAMED_CONSEQUENCE_THEN().getText());
+ String body = getTokenTextPreservingWhitespace(namedConsequnceCtx.namedConsequenceBody(), tokenStream);
Review Comment:
```suggestion
.forEach(namedConsequenceCtx -> {
String name = extractNamedConsequenceName(namedConsequenceCtx.RHS_NAMED_CONSEQUENCE_THEN().getText());
String body = getTokenTextPreservingWhitespace(namedConsequenceCtx.namedConsequenceBody(), tokenStream);
```
##########
drools-drl/drools-drl-parser/src/main/antlr4/org/drools/drl/parser/antlr4/DRLParser.g4:
##########
@@ -488,9 +508,13 @@ lhsAccumulate : (DRL_ACCUMULATE|DRL_ACC) LPAREN lhsAndDef (COMMA|SEMI)
RPAREN (SEMI)?
;
-rhs : DRL_THEN consequence ;
+rhs : DRL_THEN defaultConsequence namedConsequence* ;
+
+defaultConsequence : ( RHS_STRING_LITERAL | RHS_CHUNK )* ;
-consequence : ( RHS_STRING_LITERAL | RHS_CHUNK )* ;
+// THEN LEFT_SQUARE ID RIGHT_SQUARE chunk
+namedConsequence : RHS_NAMED_CONSEQUENCE_THEN namedConsequenceBody ;
+namedConsequenceBody : ( RHS_STRING_LITERAL | RHS_CHUNK )* ;
Review Comment:
```suggestion
namedConsequence : RHS_NAMED_CONSEQUENCE_THEN defaultConsequence ;
```
`namedConsequenceBody` is identical to `defaultConsequence` so why not reuse it?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools-5716] [new-parser] mismatched input 'if'/'do' (… [incubator-kie-drools]
Posted by "yurloc (via GitHub)" <gi...@apache.org>.
yurloc commented on PR #5831:
URL: https://github.com/apache/incubator-kie-drools/pull/5831#issuecomment-2049302157
Overall fixing progress: There were 375 failed tests.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools-5716] [new-parser] mismatched input 'if'/'do' (… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on code in PR #5831:
URL: https://github.com/apache/incubator-kie-drools/pull/5831#discussion_r1556999614
##########
drools-drl/drools-drl-parser/src/main/java/org/drools/drl/parser/antlr4/Antlr4ParserStringUtils.java:
##########
@@ -66,13 +66,13 @@ public static String getTokenTextPreservingWhitespace(ParserRuleContext ctx, Tok
}
/**
- * Just remove leading "then"
+ * Extract name from "then[name]" of RHS_NAMED_CONSEQUENCE_THEN
*/
- public static String trimThen(String rhs) {
- if (rhs.startsWith("then")) {
- return rhs.substring("then".length());
+ public static String extractNamedConsequenceName(String namedConsequenceThen) {
+ if (namedConsequenceThen.toLowerCase().startsWith("then[") && namedConsequenceThen.endsWith("]")) {
+ return namedConsequenceThen.substring("then[".length(), namedConsequenceThen.length() - 1);
Review Comment:
The String manipulation is not very nice, but if we split `RHS_NAMED_CONSEQUENCE_THEN` into finer-grained tokens, Lexer and Parser would get more complex while we want to retrieve RHS as simple text as possible.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools-5716] [new-parser] mismatched input 'if'/'do' (… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on code in PR #5831:
URL: https://github.com/apache/incubator-kie-drools/pull/5831#discussion_r1558768229
##########
drools-drl/drools-drl-parser/src/main/antlr4/org/drools/drl/parser/antlr4/DRLLexer.g4:
##########
@@ -172,6 +172,8 @@ RHS_STRING_LITERAL
| ('\'' ( DrlEscapeSequence | ~('\\'|'\'') )* '\'') { setText( normalizeString( getText() ) ); }
;
+RHS_NAMED_CONSEQUENCE_THEN : 'then' LBRACK IDENTIFIER RBRACK ;
Review Comment:
@yurloc Thanks!
##########
drools-drl/drools-drl-parser/src/main/java/org/drools/drl/parser/antlr4/DRLVisitorImpl.java:
##########
@@ -292,8 +294,17 @@ public RuleDescr visitRuledef(DRLParser.RuledefContext ctx) {
}
if (ctx.rhs() != null) {
+ // default consequence
ruleDescr.setConsequenceLocation(ctx.rhs().getStart().getLine(), ctx.rhs().getStart().getCharPositionInLine()); // location of "then"
- ruleDescr.setConsequence(trimThen(getTokenTextPreservingWhitespace(ctx.rhs(), tokenStream))); // RHS is just a text
+ ruleDescr.setConsequence(getTokenTextPreservingWhitespace(ctx.rhs().defaultConsequence(), tokenStream)); // RHS is just a text
+
+ // named consequences
+ ctx.rhs().namedConsequence()
+ .forEach(namedConsequnceCtx -> {
+ String name = extractNamedConsequenceName(namedConsequnceCtx.RHS_NAMED_CONSEQUENCE_THEN().getText());
+ String body = getTokenTextPreservingWhitespace(namedConsequnceCtx.namedConsequenceBody(), tokenStream);
Review Comment:
@yurloc Thanks!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools-5716] [new-parser] mismatched input 'if'/'do' (… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas commented on PR #5831:
URL: https://github.com/apache/incubator-kie-drools/pull/5831#issuecomment-2044247906
@yurloc @gitgabrio @mariofusco Please review, thanks!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [incubator-kie-drools-5716] [new-parser] mismatched input 'if'/'do' (… [incubator-kie-drools]
Posted by "tkobayas (via GitHub)" <gi...@apache.org>.
tkobayas merged PR #5831:
URL: https://github.com/apache/incubator-kie-drools/pull/5831
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org