Added support for string parameters and variables #56

LuigiPulcini · 2024-04-19T11:44:55Z

If there is any interest in adding support for string parameters and variables, this PR adjusts the version forked by @issidorov (https://github.com/issidorov/fparse/), updating it to the latest master branch (including the refactoring to Typescript).

This is also requested in #51.

An example of what this version can do is offered below:

const f1 = new Formula( 'if( compare( [quantity], 10, ">=" ), .9, 1 ) * [price]' );

f1.if = ( value, ifTrue, ifFalse ) => {
    return ! ! value ? ifTrue : ifFalse;
};

f1.compare = ( value, reference, comparison ) => {
    switch ( comparison ) {
        case '>=':
            return value >= reference ? 1 : 0;
        ...
        case '>':
        default:
            return value > reference ? 1 : 0;
    }
};

const res = f1.evaluate( { quantity: 10, price: 100 } );
console.log( res );

// res = 900

I hope this helps.

bylexus

Thanks for your contribution. I started the review by checking out your branch, building it (using nodejs 20), and ran the tests (npm run test). There are 13 tests failing.

Would you have a look into it, or maybe I did oversee something?
Thanks

bylexus · 2024-04-20T12:36:20Z

spec/specs/stringBasicSpec.js

+    beforeEach(function () {
+        if (typeof require !== 'undefined') {
+            Fparser = require('../../dist/fparser-dev');
+        } else {
+            Fparser = window.Formula;
+        }
+    });


The CommonJS syntax is no longer needed / supported since the switch to a TypeScript library. Please remove this require "hack". (see for example spec/specs/basicSpec.js).

This seems not to be fixed - when executing the tests with npm run test, the window object is not present - please remove the whole beforeEach() function, as it is no longer needed since the TypeScript build.

bylexus

I am very sorry, but still, most of the tests fail when running npm run test. Maybe I am missing something?

Please correct either the failing code, or fix the tests (one of the two seems not correct).

bylexus · 2024-04-21T15:55:27Z

spec/specs/stringBasicSpec.js

+    beforeEach(function () {
+        if (typeof require !== 'undefined') {
+            Fparser = require('../../dist/fparser-dev');
+        } else {
+            Fparser = window.Formula;
+        }
+    });


This seems not to be fixed - when executing the tests with npm run test, the window object is not present - please remove the whole beforeEach() function, as it is no longer needed since the TypeScript build.

bylexus · 2024-04-21T15:58:14Z

spec/specs/stringBasicSpec.js

+        });
+    });
+
+    it('blocking math functions', function () {


this test fails.

bylexus · 2024-04-21T15:58:34Z

spec/specs/stringBasicSpec.js

+        });
+    });
+
+    it('blocking math operator "^"', function () {


this test fails.

bylexus · 2024-04-21T15:59:28Z

spec/specs/stringBasicSpec.js

+        expect(Fparser.calc('myFnFoo([myVar])', { myFnFoo: myFnFoo, myVar: 'bar' })).toEqual('foobar');
+    });
+
+    it('blocking math operator "*" of number and variable', function () {


this test fails.

bylexus · 2024-04-21T16:00:05Z

spec/specs/stringBasicSpec.js

+    });
+
+    it('correct in parentheses', function () {
+        expect(Fparser.calc('(")")')).toEqual(')');


this test fails.

bylexus · 2024-04-21T16:01:55Z

spec/specs/stringBasicSpec.js

+        expect(() => Fparser.calc('2x', { x: 'foo' })).toThrowError(expectedError);
+    });
+
+    it('blocking math operator "+"', function () {


this test fails.

bylexus · 2024-04-21T16:02:09Z

spec/specs/stringBasicSpec.js

+        expect(Fparser.calc('"   "')).toEqual('   ');
+    });
+
+    it('allow special chars', function () {


this test fails.

bylexus · 2024-04-21T16:02:22Z

spec/specs/stringBasicSpec.js

+        });
+    });
+
+    it('blocking math operator "/"', function () {


this test fails.

bylexus · 2024-04-21T16:02:36Z

spec/specs/stringBasicSpec.js

+        });
+    });
+
+    it('blocking math operator "*"', function () {


this test fails.

bylexus · 2024-04-21T16:02:49Z

spec/specs/stringBasicSpec.js

+        expect(Fparser.calc('[myVar]', { myVar: 'foo' })).toEqual('foo');
+    });
+
+    it('support usage by custom function', function () {


this test fails.

LuigiPulcini · 2024-04-21T18:16:51Z

Hi, @bylexus.

First of all, thank you so much for promptly reviewing my pull request. I am sorry I completely missed the fact that the tests would all fail with the library refactoring in Typescript.

I have now adjusted a few more details in the library and fixed all the tests accordingly. When I run npm run test, everything runs smoothly, now.

Adding string support Merge branch 'barn2plugins-master' into develop

bylexus · 2024-04-23T19:33:44Z

Hi @LuigiPulcini,

Thanks for your contribution, this is a valuable addition. I did some adaptions to your original pull request, mostly type definitions and some documentation.

For the time being, I merged it back to the develop branch, I want to implement some additional changes before merging it to the main and creating a version, namely:

support for double- and single quote strings (now: only double quotes)
make parser state names via enum, instead of error-prone strings

As soon as I added those additions, I will publish a new version.

added support for string parameters and variables

6bcda83

bylexus mentioned this pull request Apr 19, 2024

string params #51

Open

bylexus changed the base branch from master to develop April 20, 2024 12:18

bylexus reviewed Apr 20, 2024

View reviewed changes

added support for single- and double-quoted strings

06ce135

bylexus requested changes Apr 21, 2024

View reviewed changes

LuigiPulcini added 2 commits April 21, 2024 20:10

fixed string validation

4df63bc

fixed string spec tests

1c88fea

LuigiPulcini requested a review from bylexus April 21, 2024 18:13

bylexus added a commit that referenced this pull request Apr 23, 2024

Merging pull request #56

ea6ea99

Adding string support Merge branch 'barn2plugins-master' into develop

bylexus merged commit 1c88fea into bylexus:develop Apr 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for string parameters and variables #56

Added support for string parameters and variables #56

LuigiPulcini commented Apr 19, 2024 •

edited

Loading

bylexus left a comment

bylexus Apr 20, 2024

bylexus Apr 21, 2024

bylexus left a comment

bylexus Apr 21, 2024

bylexus Apr 21, 2024

bylexus Apr 21, 2024

bylexus Apr 21, 2024

bylexus Apr 21, 2024

bylexus Apr 21, 2024

bylexus Apr 21, 2024

bylexus Apr 21, 2024

bylexus Apr 21, 2024

bylexus Apr 21, 2024

LuigiPulcini commented Apr 21, 2024

bylexus commented Apr 23, 2024

Added support for string parameters and variables #56

Added support for string parameters and variables #56

Conversation

LuigiPulcini commented Apr 19, 2024 • edited Loading

bylexus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bylexus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LuigiPulcini commented Apr 21, 2024

bylexus commented Apr 23, 2024

LuigiPulcini commented Apr 19, 2024 •

edited

Loading