Skip to content

Conversation

@jbee
Copy link
Collaborator

@jbee jbee commented Oct 28, 2025

The function d2:condition was defined in the parser but not implemented. This function is very similar to the if function with the difference that the condition is quoted (for example d2:condition('((1 == 1) && (2 == 2))',1,0)). The implementation uses the same evaluation as for the if function after the condition expression is unquoted.

Related ticket.

@jbee jbee self-assigned this Oct 28, 2025
*/
object ExpressionGrammar {

private val STRING_PARSED = Fragment {expr, ctx -> run {
Copy link
Member

Choose a reason for hiding this comment

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

I think the run function is not necessary here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IDK kotlin well enough, this was a suggestion by the IDE to add it. Do you think I should remove it?

Copy link
Member

Choose a reason for hiding this comment

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

I think it has no effect in this particular case, but maybe there is something else I don't see. The run method executes a block of code as an expression (check the last paragraph in this section). In this case, this part of the code builds a Fragment object by implementing the abstract method in the interface, which is this lambda function. So I don't see any difference in adding run in the body. It is fine to leave it if we want.


private val STRING_PARSED = Fragment {expr, ctx -> run {
val nestedExpr = Literals.parseString(expr); // consumes the literal in parent expr
Expr.parse(nestedExpr, ctx, false);
Copy link
Member

Choose a reason for hiding this comment

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

The ending ; are not incorrect, but not strictly necessary in kotlin.

@superskip superskip requested a review from vgarciabnz October 29, 2025 13:46
@vgarciabnz
Copy link
Member

Added a small description just for the record.

This PR looks good to me. The only missing thing is the version upgrade, it should be updated if we want a new release for this.

vgarciabnz
vgarciabnz previously approved these changes Nov 13, 2025
@sonarqubecloud
Copy link

@jbee jbee merged commit c3383b7 into main Nov 13, 2025
4 checks passed
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.

4 participants