bazel-skylib icon indicating copy to clipboard operation
bazel-skylib copied to clipboard

Add flatten function that collapses first dimension of an iterable into a list.

Open SamiKalliomaki opened this issue 3 years ago • 7 comments

SamiKalliomaki avatar Nov 15 '22 15:11 SamiKalliomaki

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

aiuto avatar Nov 15 '22 15:11 aiuto

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
]]

SamiKalliomaki avatar Nov 15 '22 16:11 SamiKalliomaki

What are you actually trying to accomplish with that?

On Tue, Nov 15, 2022 at 11:36 AM Sami Kalliomäki @.***> wrote:

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 ]]

— Reply to this email directly, view it on GitHub https://github.com/bazelbuild/bazel-skylib/pull/413#issuecomment-1315573644, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXHHHBYFTZQJURK73GSL3LWIO3Y7ANCNFSM6AAAAAASBCHZ2M . You are receiving this because you commented.Message ID: @.***>

aiuto avatar Nov 15 '22 16:11 aiuto

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 avatar Nov 15 '22 17:11 SamiKalliomaki

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

SamiKalliomaki avatar Nov 16 '22 10:11 SamiKalliomaki

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?

aiuto avatar Nov 21 '22 20:11 aiuto

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?

motiejus avatar Dec 13 '22 14:12 motiejus