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

Rules element with Rules and Group by #19

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Rikkola
Copy link
Contributor

@Rikkola Rikkola commented Oct 31, 2024

Examples of the added feature are in yard-core/test/rules folder.

Few notes about this.

  • This adds a Store type for YARD inputs. Other than that the input type is not really used ATM.
  • Previously every type used in YARD elements and as YARD outputs was a boolean, number, string, map and list. This adds Date and Duration.
    • To get a date there is a date() function added to MVEL. Style borrowed from FEEL.
  • sum and count as groupby accumulator functions. Sum also accepts parameters that can be resolved with MVEL.

Things to add later:

  • Types DateTime, Time
  • Functions dateTime(), time()
  • Rest of the group by accumulate functions

Things to reconsider:

  • I also automatically imported java.time.Duration. However Duration as a name is a likely input name. This might not be an issue and the user can work around it or we mask Duration and give it another name.

Group by
Copy link
Contributor

@lucamolteni lucamolteni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR introduces a whole lot of Drools inside YaRD.
It does it directly, as its resulting syntax is very similar to Drools'.

The problem is that the current YaRD syntax is pretty different from Drools', as it's heavily inspired by DMN instead. So with this PR it feels like having Drools and DMN together in a single file, which seems a bit confusing.

For scorecards specifically I'm not sure we need this feature, so I wouldn't merge as it is. Perhaps I'm wrong and the users will need this from day one, but I still think we should design the YAML syntax first.

@@ -27,7 +27,8 @@
key = "type",
value = {
@YamlSubtype(alias = "DecisionTable", type = DecisionTable.class),
@YamlSubtype(alias = "LiteralExpression", type = LiteralExpression.class)
@YamlSubtype(alias = "LiteralExpression", type = LiteralExpression.class),
@YamlSubtype(alias = "Rules", type = RuleExpression.class)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly this is putting a whole Drools like syntax dialect inside the current AST of YaRD, which I'm not necessarily against but it seems very different to what we have now.

Currently there is no documentation nor specification of what YaRD should be so this seems like a risky change

import java.util.HashMap;
import java.util.Map;

public class MVELLER {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and its test) needs definitely a better name i.e.

MVELExpression
MVELThen

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.

2 participants