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 flatten function that collapses first dimension of an iterable into a list. #413

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

SamiKalliomaki
Copy link

No description provided.

@aiuto
Copy link
Contributor

aiuto commented Nov 15, 2022

This seems like a very low value thing to add. It is basically a wrapper for a one line list comprehsension.

@SamiKalliomaki
Copy link
Author

The use case I have is:

flatten([
    select({
        ":%s_enabled" % feature: ["//some/path/%s:deps" % feature],
        "//conditions:default": [],
    })
    for feature in ALL_FEATURES
])

IMO that is a lot clearer than:

[target for target in sublist for sublist in [
    select({
        ":%s_enabled" % feature: ["//some/path/%s:deps" % feature],
        "//conditions:default": [],
    })
    for feature in ALL_FEATURES
]]

@aiuto
Copy link
Contributor

aiuto commented Nov 15, 2022 via email

@SamiKalliomaki
Copy link
Author

I want to allow passing in a command line argument to allow selecting which features to include. Because the number of "features" is large (around 100 in my case) and changes often, it is not feasible to do this without for loops.

@SamiKalliomaki
Copy link
Author

Actually the list comprehension implementation does not work with selects. Updated implementation.

@aiuto
Copy link
Contributor

aiuto commented Nov 21, 2022

It's not my call, but I don't think we should be adding more functions to skylib unless they fill a clear need for many users. I won't be approving or merging this. Anyone else?

@motiejus
Copy link

To be fair, I found this PR when looking for "bazel-skylib flatten" in my favorite search engine. :)

In general, I agree with not including it as-is, because "flatten" name alone is ambiguous: it does not specify how deep of nesting it will unflatten.

E.g. this:

[1, "foo", ["bar", ["baz"]]

Can be flattened to:

  • [1, "foo", "bar", "baz"]
  • [1, "foo", "bar", ["baz"]]
  • [1, "f", "o", "o", "bar", ["baz"]
  • [1, "f", "o", "o", "b", "a", "r", "b", "a", "z"]

... and all of them will be correct by some definition of flatten.

Exercise to the reader: which result is the one in the PR?

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.

3 participants