google / jsonnet

Jsonnet - The data templating language
http://jsonnet.org
Apache License 2.0
6.92k stars 438 forks source link

feat: std.flattenDeepArray Concatenate an array containing values and arrays into a single flattened array. #1082

Closed bitmaybewise closed 1 year ago

bitmaybewise commented 1 year ago

Proposal

The function compactArray recursively flattens multiple arrays, eliminating null values.

Example:

{
  result: std.compactArray([1, [2, 3], ['4', null], []])
}

Outputs:

{
  "result": [1, 2, 3, "4"]
}
google-cla[bot] commented 1 year ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

sparkprime commented 1 year ago

It should give an error in that case. It's supposed to take an array of arrays and return an array.

bitmaybewise commented 1 year ago

It should give an error in that case. It's supposed to take an array of arrays and return an array.

@sparkprime I believe you're right. It would be misleading given the function name.

I want to propose a new function to the stdlib, compactArray. I believe it would be better to have a new function. WDYT?

sparkprime commented 1 year ago

I think you can write it more simply like this:

local old_std = std;
local std = old_std {
  flattenDeep(value):
    if std.type(value) == 'array' then
      [y for x in value for y in std.flattenDeep(x)]
    else if value == null then
      []
    else
      [value],
};

{ 
  simpleVal: std.flattenDeep(0),
  simpleArr: std.flattenDeep([1, 2, 3]),
  mix: std.flattenDeep([1, [2, 3]]),
  mixWithNulls: std.flattenDeep([null, 1, null, [null, 2, null, 3, null]]),
  deeper: std.flattenDeep([[null], [[1], null], [null, 2, [null, [3, null]], null]]),
}

But actually I wonder why exclude null by default? If you remove the part that checks for null and returns [] then it will leave the nulls in the linear array. That might be useful in some cases. If you don't want them, you can remove them with [x in std.flattenDeep(foo) if x != null].

sparkprime commented 1 year ago

BTW it wasn't clear to me if you wanted to support > 2 levels of nesting or whether the way you were recursively calling fold from within the accumulator would actually achieve that.

bitmaybewise commented 1 year ago
local old_std = std;
local std = old_std {
  flattenDeep(value):
    if std.type(value) == 'array' then
      [y for x in value for y in std.flattenDeep(x)]
    else if value == null then
      []
    else
      [value],
};

{ 
  simpleVal: std.flattenDeep(0),
  simpleArr: std.flattenDeep([1, 2, 3]),
  mix: std.flattenDeep([1, [2, 3]]),
  mixWithNulls: std.flattenDeep([null, 1, null, [null, 2, null, 3, null]]),
  deeper: std.flattenDeep([[null], [[1], null], [null, 2, [null, [3, null]], null]]),
}

@sparkprime practically speaking, that is basically my proposal, but your implementation has the added benefit that if a single value is fed, it turns it into an array of a single value. It could be useful in some cases where the input could be either null or array(s).

I also like the approach you mentioned to strip nulls: [x in std.flattenDeep(foo) if x != null]. It is an extra step compared to the compactArray proposal, but simple and easy too. Array comprehensions tend to become difficult to read and understand, but in this case I don't believe it's harmful.

I usually prefer to represent the absence of values differently, and tend to strip nulls from third party libraries outputs, or transform them before feeding to other functions, but I guess it is a preference of mine rather than a good reason to not keep them.

By simplifying the function to not handle null values makes it simpler (as you said):

local old_std = std;
local std = old_std {
  flattenDeep(value):
    if std.type(value) == 'array' then
      [y for x in value for y in std.flattenDeep(x)]
    else
      [value],
{ 
  simpleVal: std.flattenDeep(0),
  simpleArr: std.flattenDeep([1, 2, 3]),
  mix: std.flattenDeep([1, [2, 3]]),
  mixWithNulls: std.flattenDeep([null, 1, null, [null, 2, null, 3, null]]),
  deeper: std.flattenDeep([[null], [[1], null], [null, 2, [null, [3, null]], null]]),
}

So thinking that not removing nulls would serve more purposes, I believe your suggestion will be more beneficial to the stdlib.

sparkprime commented 1 year ago

In some ways [] already represents null in the sense that if you want to return an optional thing deep while building the structure, you can return [] instead of null.