EdgeVerve / feel

Expression Language for creating and executing business rules in decision table based on DMN 1.1 specification for conformance level 3
MIT License
93 stars 48 forks source link

Subtracting durations from date and time gives wrong year #11

Closed a-hegerath closed 3 years ago

a-hegerath commented 6 years ago
date and time("2018-03-01T00:00:00Z") - duration("P3M") 

is evaluated to

date and time("2019-12-01T00:00:00Z") 

instead of

date and time("2017-12-01T00:00:00Z")

This is likely due to a wrong sign in line 244 of file fn-generator.js

return dateandtime(date(x.year - (y.years + Math.floor((x.month - y.months) / 12)), (x.month - y.months) - (Math.floor((x.month - y.months) / 12) * 12), x.day), time(x));

I assume it should instead be:

return dateandtime(date((x.year - y.years) + Math.floor((x.month - y.months) / 12), (x.month - y.months) - (Math.floor((x.month - y.months) / 12) * 12), x.day), time(x));

so that the "overflow" year is subtracted, not added.

Unrelated to this, subtracting a duration from a date (without a time) fails, but I do not know if this is by design? In the statement above, time(x) fails since for a date (instead of a date time) there is no time part.

deostroll commented 6 years ago

Hey, thanks for pointing out this issue. Can you submit a PR with the test that captures this failure. Before you do so, please sign our CLA. Once this is done we will include that check in our tests.

a-hegerath commented 6 years ago

Since the test is pretty simple, maybe it is easier if I just paste it here? Like the other date comparison tests, I assume it should go in file ./test/comparision-expression/feel-comparision-expression.build.spec.js:

  it('Successfully subtract duration from date', function(done) {
        var text = 'date and time("2018-03-01T00:00:00Z") - duration("P3M") = date and time("2017-12-01T00:00:00Z")';
        var parsedGrammar = FEEL.parse(text);
        parsedGrammar.build().then(result => {
            expect(result).to.be.true;
            done();
        }).catch(err => done(err));
    });
deostroll commented 6 years ago

Hi. Thankyou. We have incorporated the change and our tests all passed too. We will roll this out in the next release, and then manually close this issue.