correct set.union method #574
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `union` function on `set` is inconsistent with the behaviour of `update` as it does not support multiple iterable positional arguments as is the case in the Bazel specification of the language. This PR will align `starlark-go` with the Bazel spec. Note that the `set.union` method no longer uses the `Union` function defined in `value.go` in order to avoid making a new `set` instance for each interable processed.
adonovan
reviewed
Jan 28, 2025
starlark/library.go
Outdated
| defer iter.Done() | ||
| return resultSet.InsertAll(iter) | ||
| }(); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
This should return nameErr, like set_update.
But instead let's factor the common code of set_update and set_union since they only differ by whether the destination is a clone:
func set_union(_ *Thread, b *Builtin, args Tuple, kwargs []Tuple) (Value, error) {
if len(kwargs) > 0 {
return nil, nameErr(b, "union does not accept keyword arguments")
}
res := b.Receiver().(*Set).clone()
if err := setUpdate(res, args); err != nil {
return nil, nameErr(b, err)
}
return res, nil
}
func set_update(_ *Thread, b *Builtin, args Tuple, kwargs []Tuple) (Value, error) {
if len(kwargs) > 0 {
return nil, nameErr(b, "update does not accept keyword arguments")
}
recv := b.Receiver().(*Set)
if err := setUpdate(res, args); err != nil {
return nil,nameErr(b, err)
}
return None, nil
}
There was a problem hiding this comment.
Ah true; I will do that...
There was a problem hiding this comment.
I've also factored-out the check for the kwargs because the update / union prefix can be added later albeit with an additional colon in the message. This seems OK to me; let me know what you think.
Fixes in PR from @adonovan.
function name change
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
unionfunction onsetis inconsistent with the behaviour ofupdateas it does not support multiple iterable positional arguments as is the case in the Bazel specification of the language. This PR will alignstarlark-gowith the Bazel spec.Note that the
set.unionmethod no longer uses theUnionfunction defined invalue.goin order to avoid making a newsetinstance for each interable processed.