Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

SPR-9164 - Added BigDecimal capabilities to SpEl numeric operators #80

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

giovannidalloglio commented May 15, 2012

Clean version of old pull request #54

This should cover part of SPR-9164

[added BigDecimal support to SpEl numerics calculations.]

PS: I have signed and agree to the terms of the SpringSource Individual Contributor License Agreement.

Contributor

cbeams commented May 15, 2012

In addition to the contributor guidelines, you might find it helpful, Giovanni, to simply browse through the commit log to see how other commits look. You can easily do this with git log, and particularly with git log --author=cbeams

Contributor

giovannidalloglio commented May 21, 2012

I amended my work, rebasing on current Spring's master.

I hope that this'll be the right time.

@cbeams: you commented my choice:

performance: the if block for Double should come first, then the else if for BigDecimal. The former is more likely to be used than the latter, meaning that this arrangement will result in fewer cycles being used up.

This is a choice: to not risk the calculation error due to the double approximation (is this the same error that this issue want to fix?). Using the BigDecimal as first choice, we get away from this risk.

Do you agree?

Contributor

obecker commented Jul 1, 2012

@cbeams @giovannidalloglio regarding test for Double first or test for BigDecimal first:

If one operand is Double and the other is BigDecimal, then the operation should treat both as BigDecimal. So the test for BigDecimal first is correct.

@ghost ghost assigned cbeams Jul 2, 2012

@aclement aclement commented on an outdated diff Nov 13, 2013

...rg/springframework/expression/spel/OperatorTests.java
+ evaluate("3 le 5", true, Boolean.class);
+ evaluate("5 le 3", false, Boolean.class);
evaluate("3 le 5", true, Boolean.class);
evaluate("5 le 3", false, Boolean.class);
@aclement

aclement Nov 13, 2013

Contributor

These new two lines are just a duplicate of the two lines preceding them??

@aclement aclement commented on an outdated diff Nov 13, 2013

...rg/springframework/expression/spel/OperatorTests.java
}
@Test
public void testEqual() {
+ evaluate("3 == 5", false, Boolean.class);
+ evaluate("5 == 3", false, Boolean.class);
evaluate("3 == 5", false, Boolean.class);
evaluate("5 == 3", false, Boolean.class);
@aclement

aclement Nov 13, 2013

Contributor

Same here, two new lines duplicating the two lines above them.

@aclement aclement commented on an outdated diff Nov 13, 2013

...rg/springframework/expression/spel/OperatorTests.java
+ evaluate("3 eq 5", false, Boolean.class);
+ evaluate("5 eQ 3", false, Boolean.class);
evaluate("3 eq 5", false, Boolean.class);
evaluate("5 eQ 3", false, Boolean.class);
@aclement

aclement Nov 13, 2013

Contributor

and again...

@aclement aclement commented on an outdated diff Nov 13, 2013

...rg/springframework/expression/spel/OperatorTests.java
}
@Test
public void testNotEqual() {
+ evaluate("3 != 5", true, Boolean.class);
+ evaluate("5 != 3", true, Boolean.class);
evaluate("3 != 5", true, Boolean.class);
evaluate("5 != 3", true, Boolean.class);
@aclement

aclement Nov 13, 2013

Contributor

and again...

@aclement aclement commented on an outdated diff Nov 13, 2013

...rg/springframework/expression/spel/OperatorTests.java
+ evaluate("3 ne 5", true, Boolean.class);
+ evaluate("5 nE 3", true, Boolean.class);
evaluate("3 ne 5", true, Boolean.class);
evaluate("5 nE 3", true, Boolean.class);
@aclement

aclement Nov 13, 2013

Contributor

...

@aclement aclement commented on an outdated diff Nov 13, 2013

...rg/springframework/expression/spel/OperatorTests.java
}
@Test
public void testGreaterThanOrEqual() {
+ evaluate("3 >= 5", false, Boolean.class);
+ evaluate("5 >= 3", true, Boolean.class);
evaluate("3 >= 5", false, Boolean.class);
evaluate("5 >= 3", true, Boolean.class);
@aclement

aclement Nov 13, 2013

Contributor

...

@aclement aclement commented on an outdated diff Nov 13, 2013

...rg/springframework/expression/spel/OperatorTests.java
+ evaluate("3 GE 5", false, Boolean.class);
+ evaluate("5 gE 3", true, Boolean.class);
evaluate("3 GE 5", false, Boolean.class);
evaluate("5 gE 3", true, Boolean.class);
@aclement

aclement Nov 13, 2013

Contributor

...

@aclement aclement commented on an outdated diff Nov 13, 2013

...rg/springframework/expression/spel/OperatorTests.java
}
@Test
public void testGreaterThan() {
evaluate("3 > 5", false, Boolean.class);
evaluate("5 > 3", true, Boolean.class);
+ evaluate("3 > 5", false, Boolean.class);
+ evaluate("5 > 3", true, Boolean.class);
@aclement

aclement Nov 13, 2013

Contributor

...

@aclement aclement commented on an outdated diff Nov 13, 2013

...rg/springframework/expression/spel/OperatorTests.java
+ evaluate("3.0d gt 5.0d", false, Boolean.class);
+ evaluate("5.0d gT 3.0d", true, Boolean.class);
evaluate("3.0d gt 5.0d", false, Boolean.class);
evaluate("5.0d gT 3.0d", true, Boolean.class);
@aclement

aclement Nov 13, 2013

Contributor

and again

Contributor

aclement commented Nov 13, 2013

Apart from the duplicate tests this looks ok to me. Two further comments:

  1. The StandardTypeComparator could also be changed to include the BigDecimal case, for consistency, although i'm not sure how much that class gets exercised in practice.

  2. One point on style is that I think the spring way is to have your if statements like this:

if (foo) {
}
else if (op1 instanceof Float || op2 instanceof Float) {

rather than

if (foo) {
} else if (op1 instanceof Float || op2 instanceof Float) {

and this PR uses the latter.

Add BigDecimal support for SpEl numeric operations
Issue: SPR-9164

Prior to this change, SpEL supported numeric operations for int, float, ...
'new java.math.BigDecimal('0.1') > 0' evaluates to false
(BigDecimal is truncated to int)

This commit introduces support for BigDecimal operations
'new java.math.BigDecimal('0.1') > 0' evaluates to true
(the comparison is made with bigDecimals)

In the meantime (from the pull request to now), SpEL advanced [operator++, between].
Now is everything covered, updated and fixed.

In addition, some polish (like Phil Webb commit e83bdda)

Hello.
Everything is now OK, plus some additions:

  • From the original pull request to now, SpEL advanced [operator++, between].
    • Now is everything covered (the new features), updated and tested.
  • In addition, some polish (like Phil Webb commit e83bdda)

philwebb pushed a commit that referenced this pull request Nov 21, 2013

Merge pull request #80 from giovannidalloglio/SPR-9164
* SPR-9164:
  Add BigDecimal support for SpEl numeric operations
Owner

philwebb commented Nov 21, 2013

Great pull request! This has now been merged into master with a few minor tweaks.

Thanks!

@philwebb philwebb closed this Nov 21, 2013

rwinch pushed a commit to rwinch/spring-framework that referenced this pull request Jun 20, 2016

Polish HttpRequestBuilder API
This commit makes messageEncoders a required argument for building a
client request - those are needed to actually encode the body object as
a reactive stream to be written to the HTTP request body.

Removed raw types usage in DefaultHttpRequestBuilder.

DefaultHttpRequestBuilder now uses a UriTemplateHandler to expand URI
templates + variables into a concrete URI.

Fixes #80, fixes #85, fixes #86

rwinch pushed a commit to rwinch/spring-framework that referenced this pull request Jul 13, 2016

Polish HttpRequestBuilder API
This commit makes messageEncoders a required argument for building a
client request - those are needed to actually encode the body object as
a reactive stream to be written to the HTTP request body.

Removed raw types usage in DefaultHttpRequestBuilder.

DefaultHttpRequestBuilder now uses a UriTemplateHandler to expand URI
templates + variables into a concrete URI.

Fixes #80, fixes #85, fixes #86
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment