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

Support line breaks for FEEL statements #879

Closed
3 tasks
MaxTru opened this issue Feb 13, 2023 · 5 comments · Fixed by #938
Closed
3 tasks

Support line breaks for FEEL statements #879

MaxTru opened this issue Feb 13, 2023 · 5 comments · Fixed by #938
Assignees
Labels
bug Something isn't working

Comments

@MaxTru
Copy link
Contributor

MaxTru commented Feb 13, 2023

Is your feature request related to a problem? Please describe.

When writing FEEL expressions, it often is handy to split them across multiple lines.

For example:

myVariable = null
or
myVariable > 5

rather than

myVariable = null or myVariable > 5

which is hard to read.

  • In particular in the Web Modeler, where the prop panel can not be extended, but this is a different topic.

Screenshot_20230213_102420

When writing multiline statements, the modeler explicitly persists the lineBreaks. E.g.,:

"=myVariable = null 
or 
myVariable > 5"

According to my local testing, this works just fine with the Engine.

However, the Modeler itself will expliticly complain, that this statement is not valid.

image

Describe the solution you'd like

  • Explicitly test that multi-line FEEL is accepted by the engine (my fuzzy testing reveiled: yes). Potentially agree this as interface with engine.
  • Adjust linting and code support for FEEL to accept multi-line statements

Describe alternatives you've considered

  • Keep as is (this would make sense if multi-line is explicitly discouraged

Additional context

@MaxTru MaxTru added the enhancement New feature or request label Feb 13, 2023
@nikku
Copy link
Member

nikku commented Feb 13, 2023

Thanks for opening this issue. What you found is a grammar bug.

The same statement works properly, if indented or wrapped in brackets:

(
  myVariable = null 
  or
  myVariable > 5
)

@nikku nikku added bug Something isn't working and removed enhancement New feature or request labels Feb 13, 2023
@nikku nikku self-assigned this Feb 13, 2023
@nikku nikku added the ready Ready to be worked on label Feb 14, 2023 — with bpmn-io-tasks
@nikku nikku removed the ready Ready to be worked on label Jun 12, 2023
@nikku nikku removed their assignment Jun 12, 2023
@nikku nikku added the backlog Queued in backlog label Jun 12, 2023
@nikku
Copy link
Member

nikku commented Jun 12, 2023

Moving this back to backlog for the time being. I was not able to follow-up.

@nikku nikku self-assigned this Jul 4, 2023
@nikku nikku added ready Ready to be worked on and removed backlog Queued in backlog labels Jul 4, 2023
@nikku
Copy link
Member

nikku commented Jul 4, 2023

I'm taking a timeboxed stab at this.

@nikku nikku added the in progress Currently worked on label Jul 4, 2023 — with bpmn-io-tasks
@nikku nikku removed the ready Ready to be worked on label Jul 4, 2023
@nikku
Copy link
Member

nikku commented Jul 4, 2023

I made significant progress on this issue today but was not able to fully complete it.

Under the hood this is actually a non-trivial topic. For historical reasons, being tricked by old versions of the DMN spec lezer-feel would recognize a list of expressions, rather than a single expression. Newlines would sometimes confuse the parse as it needs to always decide if this is a continuation of an existing expression or the start of a new expression.

The solution we chose is to put the parser in full DMN FEEL 1.5 compat mode, parsing single expressions only (nikku/lezer-feel@6389057#diff-3410025680394470cd4d356b9dad20b8a39c090592049243a50866cb6d556677, https://github.com/nikku/lezer-feel/blob/master/CHANGELOG.md#100). This implied downstream changes in all supporting libraries.

What is missing is integration into our editing stack:

nikku added a commit to bpmn-io/properties-panel that referenced this issue Jul 5, 2023
@nikku nikku added fixed upstream Requires integration of upstream change and removed in progress Currently worked on labels Jul 5, 2023
@nikku
Copy link
Member

nikku commented Jul 5, 2023

Fixed upstream via bpmn-io/properties-panel#254.

fake-join bot pushed a commit to bpmn-io/properties-panel that referenced this issue Jul 5, 2023
marstamm added a commit that referenced this issue Jul 5, 2023
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed fixed upstream Requires integration of upstream change labels Jul 5, 2023
marstamm added a commit that referenced this issue Jul 6, 2023
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants