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

Added | string #5585

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

Conversation

RashmitTopG
Copy link

What changes are being made and why?

Added a new StringFilter to the Kestra project, which converts numeric inputs and string representations of numbers into their string form. This feature enhances template flexibility by allowing users to handle numeric values as strings easily.
This PR closes #5528 .


How the changes have been QAed?

Example flow code demonstrating the use of the string filter in a Pebble template

template: |

  {{ 123 | string }}   # Expected output: "123"
  {{ "456.78" | string }}  # Expected output: "456.78"
  {{ "invalid" | string }}  # Expected to raise a PebbleException

This example shows how the new StringFilter behaves with different input types.


@RashmitTopG
Copy link
Author

@anna-geller I've submitted a pull request adding a StringFilter to the Kestra project that converts numbers to strings. Please review and suggest improvements if any .

@RashmitTopG
Copy link
Author

@MilosPaunovic can you please review this PR !!

@MilosPaunovic
Copy link
Member

Nope, Java is not my strong suit. @Skraye will take a look when he gets the chance.

Comment on lines +1 to +2
package io.kestra.core.runners.pebble.filters;

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a test file given that it is in the test folder?
The contents look to be duplicate as present in the above file.

@@ -56,7 +56,8 @@ public List<BinaryOperator> getBinaryOperators() {
List<BinaryOperator> operators = new ArrayList<>();

operators.add(new BinaryOperatorImpl("??", 120, NullCoalescingExpression::new, NORMAL, Associativity.LEFT));
operators.add(new BinaryOperatorImpl("???", 120, UndefinedCoalescingExpression::new, NORMAL, Associativity.LEFT));
operators.add(
Copy link
Member

Choose a reason for hiding this comment

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

Noise, remove it

import java.util.List;
import java.util.Map;

public class StringFilter implements Filter {
Copy link
Member

Choose a reason for hiding this comment

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

As Shruti said, this is a duplicate of the class, please remove this file and implement a proper test

Comment on lines +19 to +48
@Override
public Object apply(Object input, Map<String, Object> args, PebbleTemplate self, EvaluationContext context,
int lineNumber) throws PebbleException {
if (input == null) {
return null;
}

// Handle numeric types (Integer, Long, Float, Double, BigDecimal, BigInteger)
if (input instanceof Number) {
return input.toString(); // Convert numeric input to string
}

// Handle string inputs that represent numbers (parse them as numbers and return
// their string form)
if (input instanceof String) {
try {
return new BigDecimal((String) input).toString(); // Convert the string representation of a number to
// string form
} catch (NumberFormatException e) {
throw new PebbleException(null,
"'string' filter expects a valid number. Received an invalid numeric string: " + input,
lineNumber, self.getName());
}
}

// If the input is not a number or numeric string, throw an exception
throw new PebbleException(null,
"'string' filter expects a Number (INT, FLOAT, DOUBLE, etc.) or numeric String. Received: "
+ input.getClass().getName(),
lineNumber, self.getName());
Copy link
Member

Choose a reason for hiding this comment

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

The idea is good, but you focused too much on numbers :

  • When the filter is applied on a String, it should not fail, it just does nothing
  • The filter string could be applied to anything, as every Java object has the method .toString()

Line 35 you're taking a String input, you cast int to a number to then cast it to String, it does not make sense

@Skraye
Copy link
Member

Skraye commented Nov 12, 2024

@RashmitTopG Any update on this?

@MilosPaunovic
Copy link
Member

Hi @RashmitTopG, are there any updates on this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To review
Development

Successfully merging this pull request may close these issues.

Provide a | string filter
4 participants