-
Notifications
You must be signed in to change notification settings - Fork 163
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
support dict concatenation: dict | dict #14
Comments
It is very convenient, and there's no other way that I know of it compute dict+dict in an expression. |
I really liked
@laurentlb What's the new idiomatic way to write this now? Please I hope the answer is not that I should have a new dependency on bazel-skylib. |
As @alandonovan pointed out, you can use the dict constructor with keyword args.
|
Not only does that only work when keys are strings, but the keys have to be valid identifiers. Is there any other workaround people use for this? |
Starlark could allow Python multiple kwargs in dict literal:
|
BTW would this issue cover concatenation of selects of dicts? E.g. something like: my_rule(
name = "foo",
some_attr = {}
+ select({"this": {"some": "dict"}, "default": {"other": "dict"})
+ select({"that": {"some2": "dict2"}, "default": {"other2", "dict2"}),
) This is particularly useful when doing bazel file generation. I know one can programmatically merge the selects into one, but the above would be very convenient to remove friction. |
Arguing against myself in note 2, dict+dict may be convenient, but it's also easy to write a helper function that does it:
So I tend to agree with Laurent that this doesn't need to be in the core language. Load skylib, or write this helper, as you prefer. |
The workaround is fair, though I should point out that python 3.9 just got released and now supports the union operator for dicts with Now that the feature would have consistency with python, I feel it might tip the balance (also the name of the issue should probably be renamed to |
Given the Python precedent, this is fine by me. @brandjon? |
Yes, I think dict concatenation syntax that follows Python's precedent is fine. I assume Python's semantics are that the second dict's entries overwrite the first, and that we're not deviating from that. I also assume there's no way the use of the |
@brandjon What would I have to do to get this operator into Starlark/Bazel? I have a draft PR realizing it in Bazel ready at bazelbuild/bazel#14540. The tests for dict in this repo appear to be different from those in the Bazel tree though and there haven't been any proposals since 2018. |
Will we need to update buildifier and buildtools? |
It might be the case that buildifier and buildtools don't need to be updated: The spec would of course need to be updated. In general, I am willing to carry out the work needed if it's clear enough that it will find a reviewer and be accepted. |
Alas, if you look through the PR's, there are many janitorial ones that haven't been accepted (things as simple as removing trailing whitespace or fixing links), which suggests to me that there is no one approving/reviewing. That's very sad, as this is a really good resource and I work on starlark-rust which follows this standard. |
I am also worried about this. The current issue is labeled P4, which over at the main Bazel repo would mean that it could be difficult to find a reviewer. @aiuto Do you know what the state of this repo is? Would it be likely to find a reviewer? |
@brandjon Would you accept this change in Bazel? I can help with the repo maintenance if you're too busy |
@brandjon Friendly ping. If there are reasons not to accept this change, feel free to say so. |
Bazel has been supporting dictionary concatenation with the
+
operator for a long time. Python doesn't.We've deprecated the operator for a long time, mostly because of issues in Skydoc. But the original Skydoc is going away, and it's unclear to me if we should proceed with the removal of the operator (as we rely less and less on Python).
bazelbuild/bazel#6461
Any opinion?
The text was updated successfully, but these errors were encountered: