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

Add list flatten() #980

Merged
merged 4 commits into from
Aug 16, 2024
Merged

Conversation

fabriziosestito
Copy link
Contributor

Adds list flatten, fixing #352

Signed-off-by: Fabrizio Sestito <[email protected]>
Copy link

@flavio flavio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty neat, thanks!

Copy link
Collaborator

@seirl seirl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to make this recursive by default? The common case is to flatten a list of list. I feel like a recursive flatten would better be a separate .flattenRec() function.

@fabriziosestito
Copy link
Contributor Author

@seirl I took inspiration from Elixir's List.flatten.
Also, Ruby's Array.flatten is recursive by default, but it accepts an optional level argument. We could consider this behaviour, as it is more flexible.

However, I don't have a strong opinion and I am ok with what you proposed.

@seirl
Copy link
Collaborator

seirl commented Jul 13, 2024

Interesting. FWIW:

  • in Rust, std::iter::Flatten flattens a single level.
  • in Python, itertools.chain() flattens a single level.

The reason I'm mentioning this is because flatten could also be useful to flatten a list(optional(T)) into a list(T), but the semantics are more complex if you have a recursive flatten.

@fabriziosestito
Copy link
Contributor Author

@seirl Alright, I will change this to single-level flattening.

@fabriziosestito
Copy link
Contributor Author

@seirl I've followed your suggestion, now flatten flattens to a single level, while flattenDeep flattens recursively. I've preferred the deep naming as I find it clearer for the user and it is also used by lodash.

@TristonianJones
Copy link
Collaborator

/gcbrun

ext/lists.go Outdated Show resolved Hide resolved
@TristonianJones
Copy link
Collaborator

/gcbrun

Copy link
Collaborator

@TristonianJones TristonianJones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the update. I've added a few comments on how to structure this so it's safer for production rollout and works with cost estimation tools!

ext/lists.go Outdated Show resolved Hide resolved
ext/lists.go Outdated Show resolved Hide resolved
ext/lists.go Outdated Show resolved Hide resolved
@TristonianJones
Copy link
Collaborator

/gcbrun

Copy link
Collaborator

@TristonianJones TristonianJones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update the signature of single-level flatten post-merge. Thanks for the contribution and the patience!

@TristonianJones TristonianJones merged commit 8d9b9d3 into google:master Aug 16, 2024
2 checks passed
@fabriziosestito
Copy link
Contributor Author

@TristonianJones, thank you for merging. I'm currently on vacation with limited laptop access. I'll handle the cost estimation if the task is still open when I return. Regarding the function signature, shouldn't it be something like flatten<T>(list(T | list(T))) -> list(T)?

@seirl
Copy link
Collaborator

seirl commented Aug 16, 2024

@fabriziosestito the one-level .flatten() should only flatten List[List[T]], so [1, [2, 3]].flatten() should be an error.

If you use only strongly-typed CEL, all your lists need to be homogeneous, so [1, [2, 3]] doesn't make sense as a value.

@TristonianJones
Copy link
Collaborator

@seirl I took care of the signature change in a different PR. The test case will pass checking because the mixed type list is list(dyn)

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.

4 participants