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

[FR]: Add utillities from bzlparty/lib_common #495

Closed
mgred opened this issue Aug 12, 2023 · 6 comments
Closed

[FR]: Add utillities from bzlparty/lib_common #495

mgred opened this issue Aug 12, 2023 · 6 comments
Labels
enhancement New feature or request help wanted Aspect isn't prioritizing this, but the community could

Comments

@mgred
Copy link
Contributor

mgred commented Aug 12, 2023

What is the current behavior?

No response

Describe the feature

This is a proposal to add some/all functions from bzlparty/lib_common. The motivation comes from this conversation. I follow @alexeagle advice to propose the code as a new feature to collect everybody's opinion and perspective.

My personal motivation was to have a place to collect generic functions I found useful while writing rulesets.

@kormide I was told that you have done a lot of work regarding the public API and maybe could help integrating the functionality.

I would love to work with you guys and hear what you have to say.

I totally appreciate any feedback and opinion on this

@mgred mgred added the enhancement New feature or request label Aug 12, 2023
@github-actions github-actions bot added the untriaged Requires traige label Aug 12, 2023
@alexeagle
Copy link
Collaborator

  1. Xudong has good feedback on that BCR PR that we should consider whether the syntax sugar is worth it, vs. using the starlark language directly.

the readability of any code that potentially depends on it

For example your

occurs(2, [1, 2, 3]) // True
occurs("foo", ["bar", "baz"]) // False

should probably be written

2 in [1, 2, 3] # True
"foo" in ["bar", "baz"] # False

And I think len(filter(f, arr)) == 1 is easier to read than once(f, arr) especially since it allows for expressing "exactly two matching elements"

  1. Other than occurs your lib is basically functional programming idioms that are possible thanks to the addition of lambda to the language. Maybe it could be functions.bzl or lambdas.bzl

  2. We should nitpick the names from whether there's a popular idiom for python developers, so we don't accidentally introduce more differences between that and starlark

  3. We should figure out a place to "dogfood" this so we have the experience of being a user of the new API. Maybe we can refactor other code in bazel-lib to use these.

@mgred
Copy link
Contributor Author

mgred commented Aug 12, 2023

Thanks for your feedback @alexeagle

  1. Xudong has good feedback on that BCR PR that we should consider whether the syntax sugar is worth it, vs. using the starlark language directly.

the readability of any code that potentially depends on it

For example your

occurs(2, [1, 2, 3]) // True
occurs("foo", ["bar", "baz"]) // False

should probably be written

2 in [1, 2, 3] # True
"foo" in ["bar", "baz"] # False

Good point 👍 Using the in operator is definitely the better choice here and also common in Python and TS/JS. So I think occurs can be dropped 😄

And I think len(filter(f, arr)) == 1 is easier to read than once(f, arr) especially since it allows for expressing "exactly two matching elements"

True, a comparison like len(filter(f, arr)) == n gives more flexibility but may be overloaded in common cases e.g.: if empty(["foo", "bar"]) or if is_empty(["foo", "bar"]) is more fluid to read and write than always the same comparison. At least to me 😝

  1. Other than occurs your lib is basically functional programming idioms that are possible thanks to the addition of lambda to the language. Maybe it could be functions.bzl or lambdas.bzl

I imagine loading a function like load("@aspect_bazel_lib//lib:functions.bzl", "..."), and would expect this function does something with functions. All methods currently operate on lists, wouldn't it be clearer to name it after the data type it works on, like lists.bzl in this case?

  1. We should nitpick the names from whether there's a popular idiom for python developers, so we don't accidentally introduce more differences between that and starlark

The naming is very much inspired by JS array functions. Making it more compatible/similar to python is obvious as you pointed out 👍

  1. We should figure out a place to "dogfood" this so we have the experience of being a user of the new API. Maybe we can refactor other code in bazel-lib to use these.

Yep 👍

@alexeagle
Copy link
Collaborator

lists.bzl sounds right to me. @thesayyn any opinions on this one?

@alexeagle
Copy link
Collaborator

I think this is ready to implement, @mgred if you find time and interest to send a PR I'll make sure we get it merged

@alexeagle alexeagle added help wanted Aspect isn't prioritizing this, but the community could and removed untriaged Requires traige labels Aug 31, 2023
@mgred
Copy link
Contributor Author

mgred commented Aug 31, 2023

@alexeagle

alright, i'll send a PR within the next days.. then we can finetune from there

@mgred
Copy link
Contributor Author

mgred commented Sep 21, 2023

I close this issue because #512 was merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Aspect isn't prioritizing this, but the community could
Projects
None yet
Development

No branches or pull requests

2 participants