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

feat: add function is blank() to check if a string is blank #927

Merged
merged 4 commits into from
Sep 20, 2024

Conversation

skayliu
Copy link
Contributor

@skayliu skayliu commented Sep 19, 2024

Description

feat: add function is blank() to check if a string is blank

Related issues

related to camunda/camunda#22380

@skayliu skayliu requested a review from saig0 as a code owner September 19, 2024 03:33
Copy link
Member

@saig0 saig0 left a comment

Choose a reason for hiding this comment

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

@skayliu thank you for your contribution. 🎉

The changes look good. I have only a few minor comments. Please have a look. 🍪

Comment on lines 74 to 79
private def isBlankFunction = builtinFunction(
params = List("string"),
invoke = {case List(ValString(string)) =>
ValBoolean(string.isBlank)
}
)
Copy link
Member

Choose a reason for hiding this comment

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

❌ Since is blank() is a function for a string argument, we should add the function to StringBuiltinFunctions.

BooleanBuiltinFunctions is for functions with a boolean argument.

@@ -243,4 +243,27 @@ class BuiltinFunctionsTest
"Assertion failure on evaluate the expression 'list contains(assert(my_list, my_list != null, \"The condition is not true\"), 2)': The condition is not true"
)
}

"A is blank() function" should "return Boolean" in {
Copy link
Member

Choose a reason for hiding this comment

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

❌ Same as the implementation, we should add the test cases to BuiltinStringFunctionsTest.

Comment on lines 248 to 266
evaluateExpression(
expression = """ is blank("") """
) should returnResult(true)

evaluateExpression(
expression = """ is blank(" ") """
) should returnResult(true)

evaluateExpression(
expression = """ is blank("hello world") """
) should returnResult(false)

evaluateExpression(
expression = """ is blank(" hello world ") """
) should returnResult(false)

evaluateExpression(
expression = """ is blank("\t\n\r\f") """
) should returnResult(true)
Copy link
Member

Choose a reason for hiding this comment

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

🔧 We could split the test cases into:

  • should "return true if the string contains only whitespace"
  • should "return false if the string contains non-whitespace characters"

Comment on lines 248 to 250
evaluateExpression(
expression = """ is blank("") """
) should returnResult(true)
Copy link
Member

Choose a reason for hiding this comment

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

❌ Add one more test case that verifies the invocation of the function with named parameters:

evaluateExpression(
      expression = """ is blank(string: "") """
    ) should returnResult(true)

This kind of test ensures that we don't change the parameter name accidentally.

@saig0 saig0 added the follow-up: camunda docs The PR requires an update of the Camunda documentation. label Sep 20, 2024
@skayliu
Copy link
Contributor Author

skayliu commented Sep 20, 2024

Hi, @saig0 , It's done, please check again.

@saig0 saig0 self-requested a review September 20, 2024 09:22
Copy link
Member

@saig0 saig0 left a comment

Choose a reason for hiding this comment

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

@skayliu looks good. Thank you for the fast response. 🚀

@saig0 saig0 added this pull request to the merge queue Sep 20, 2024
Merged via the queue into camunda:main with commit 209b3f6 Sep 20, 2024
5 checks passed
@skayliu skayliu deleted the skayliu-is-blank branch September 23, 2024 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
follow-up: camunda docs The PR requires an update of the Camunda documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants