Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add more lenient coercing#1172

Open
ruslanhubspot wants to merge 3 commits intoHubSpot:master from
ruslanhubspot:add-more-lenient-coercing
Open

Add more lenient coercing #1172
ruslanhubspot wants to merge 3 commits intoHubSpot:master from
ruslanhubspot:add-more-lenient-coercing

Conversation

@ruslanhubspot
Copy link

@ruslanhubspot ruslanhubspot commented Apr 1, 2024

strings can be implicitly coerced to numeric values, so comparisons like:

{% if "1000" > 5%}

works. However evaluating:

{% if "1,000" > 5%}

or:

{% if "1000.25" > 5%}

throws an error. This adds some additional cleaning to allow formatted number strings to be parsed.
Notably it does not remove decimals when comparing to floats, since those are valuable and:

{% if "1000.25" > 50.0 %}

already works.

It also only removes characters after decimals if its a properly formatted decimal number (so containing no non-digit characters and only one decimal point)

Copy link
Author

Comment on lines +144 to +146
if (value.matches(".*\\..*,.*")) {
return value;
}
Copy link
Contributor

@jasmith-hs jasmith-hs Apr 1, 2024

Choose a reason for hiding this comment

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

Prefer precompiled regex expressions

Comment on lines +155 to +157
if (!value.matches("\\d*\\.\\d*")) {
return value;
}
Copy link
Contributor

@jasmith-hs jasmith-hs Apr 1, 2024

Choose a reason for hiding this comment

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

Prefer precompiled regex expressions

Comment on lines +131 to +134
if (value instanceof String) {
String sanitizedValue = trimDecimal(trimCommas((String) value));
return super.coerceToByte(sanitizedValue);
}
Copy link
Contributor

@jasmith-hs jasmith-hs Apr 1, 2024

Choose a reason for hiding this comment

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

Don't think we should do this when coercing to byte

Comment on lines +87 to +90
assertThat(
jinjava.renderForResult("{{ \"150.25,0\" is ge 4 }}", new HashMap<>()).getErrors()
)
.isNotEmpty();
Copy link
Contributor

@jasmith-hs jasmith-hs Apr 1, 2024

Choose a reason for hiding this comment

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

Some languages have decimals and commas flipped and should be handled in the opposite manner

Copy link
Author

@ruslanhubspot ruslanhubspot Apr 1, 2024

Choose a reason for hiding this comment

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

Hmm, would your suggested approach here be to make the TruthyTypeConverter constructor take in a JinjavaContext param in order to access in the current language?

Copy link
Author

@ruslanhubspot ruslanhubspot Apr 1, 2024

Choose a reason for hiding this comment

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

or perhaps just a Locale

Comment on lines +65 to +68
if (value instanceof String) {
String sanitizedValue = trimCommas((String) value);
return super.coerceToDouble(sanitizedValue);
}
Copy link
Contributor

@jasmith-hs jasmith-hs Apr 1, 2024

Choose a reason for hiding this comment

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

I'd prefer these to use similar logic as in IntFilter and FloatFilter. Make use of NumberFormat#parse

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasmith-hs jasmith-hs jasmith-hs requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Comments

AltStyle によって変換されたページ (->オリジナル) /