Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Jun 13, 2023

Conversation

bitmaybewise
Copy link
Contributor

@bitmaybewise bitmaybewise commented May 5, 2023

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
Copy link

google-cla bot commented May 5, 2023

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.

@bitmaybewise bitmaybewise marked this pull request as ready for review May 5, 2023 14:19
@bitmaybewise bitmaybewise changed the title Flat arrays should stay as is std.flattenArrays: Flat arrays should stay as is May 5, 2023
@sparkprime
Copy link
Member

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

@bitmaybewise
Copy link
Contributor Author

bitmaybewise commented May 7, 2023

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?

@bitmaybewise bitmaybewise changed the title std.flattenArrays: Flat arrays should stay as is std.compactArray: recursively flatten arrays May 7, 2023
@bitmaybewise bitmaybewise changed the title std.compactArray: recursively flatten arrays (Proposal) std.compactArray: recursively flatten arrays May 10, 2023
@bitmaybewise bitmaybewise changed the title (Proposal) std.compactArray: recursively flatten arrays feat: std.compactArray concatenates an array of arrays into a single flattened array, removing null values. May 23, 2023
@sparkprime
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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.

@bitmaybewise bitmaybewise force-pushed the flattening-flat-arrays branch 2 times, most recently from 1a5c906 to 0cd7338 Compare June 3, 2023 10:20
@bitmaybewise bitmaybewise changed the title feat: std.compactArray concatenates an array of arrays into a single flattened array, removing null values. feat: std.flattenDeepArray Concatenate an array containing values and arrays into a single flattened array. Jun 3, 2023
@sparkprime sparkprime merged commit fe8179a into google:master Jun 13, 2023
1 check passed
@sparkprime
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants