Fix chain operator to respect array constructor #7
Conversation
|
FYI: this PR also includes the changes from the other PR as they are a prerequisite for the test cases. Once the other PR got merged, that changes should not be visible in the diff anmore. |
|
Thanks @netomi , I'll take a look |
tests/custom_function_test.py
Outdated
| assert expression.evaluate(None) == [1] | ||
|
|
||
| def test_singleton_map_with_chain(self): | ||
| expression = jsonata.Jsonata("$ ~> $map($square)[]") |
There was a problem hiding this comment.
When I try the following in jsonata.js, it fails. Perhaps this is a bug in jsonata.js as well (unless I'm doing something wrong)? If so, I'll wait until it is fixed there before making the fix here.
var expr = jsonata("$ ~> $map($square)[]");
expr.registerFunction("square", function(x) {
return x * x;
});
var result = await expr.evaluate([1]);
var expected = [1];
expect(result).to.deep.equal(expected);
There was a problem hiding this comment.
indeed, I did not test the behavior of the original jsonata.js implementation. Your observation is correct, that also does not work there and the problem seems to be that the chain operator does not correctly propagate whether any child expression is forced to keep the array.
Will report the problem there with a reference to this ticket. Probably the same fix needs to be applied there.
However I want to express that without this working, the map function is pretty much useless to me in my use-case. The workaround is to always create a lambda function myself to operate on all values of an array, which is a bit of a pain. I dont see any value in a map function that returns different results depending on the input values.
| @@ -1180,6 +1180,7 @@ def lambda1(terms): | |||
| result.position = expr.position | |||
| result.lhs = self.process_ast(expr.lhs) | |||
| result.rhs = self.process_ast(expr.rhs) | |||
| result.keep_array = result.lhs.keep_array or result.rhs.keep_array | |||
There was a problem hiding this comment.
Perhaps this is the fix that is needed in jsonata.js? If so, I'll wait until the fix is made there first.
tests/custom_function_test.py
Outdated
| assert expression.evaluate(None) == [1, 4, 9] | ||
|
|
||
| def test_singleton_map(self): | ||
| expression = jsonata.Jsonata("$map([1], $square)[]") |
There was a problem hiding this comment.
This test seems to pass without any changes
netomi
force-pushed
the
support-keep-array-for-map
branch
from
d3c7584 to
85123ac
Compare
|
Created jsonata-js/jsonata#714 |
netomi
changed the title
|
The additional test cases can be removed once the PR to the upstream repo has been accepted and merged as the test cases are added to the test suite there which is already used in this project. |
|
thanks a lot for your quick action, I am really looking forward to use this library more in my projects. |
This fixes the implementation to be able to force the map function to return an array even if there is a single element.
This seems to be common for all jsonata implementations, that map returns a single element if its result is a singleton list. The commonly shared workaround is to add [] at the end of the function as explained here: jsonata-js/jsonata#462 (comment)
This was not working before, the fix propagates the keep_array setting to the parent AST element in some cases to support that. I am not sure if this is generally correct, it fixes the test cases though that I added to illustrate the problem.