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.

Sign up for GitHub

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

correct set.union method #574

Merged
merged 3 commits into from
Jan 28, 2025
Merged

correct set.union method #574

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
efb6229
correct set.union method
andponlin-canva Jan 28, 2025
84e09ee
correct set.union method
andponlin-canva Jan 28, 2025
85ef7ae
correct set.union method
andponlin-canva Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions doc/spec.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3762,9 +3762,8 @@ x.symmetric_difference([3, 4, 5]) # set([1, 2, 4, 5])
<a id='set·union'></a>
### set·union

`S.union(iterable)` returns a new set into which have been inserted
all the elements of set S and all the elements of the argument, which
must be iterable.
`S.union(iterable...)` returns a new set into which have been inserted
all the elements of set S and each element of the iterable sequences.

`union` fails if any element of the iterable is not hashable.
andponlin-canva marked this conversation as resolved.
Show resolved Hide resolved

Expand Down
28 changes: 19 additions & 9 deletions starlark/library.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2337,17 +2337,27 @@ func set_symmetric_difference(_ *Thread, b *Builtin, args Tuple, kwargs []Tuple)

// https://github.com/google/starlark-go/blob/master/doc/spec.md#set·union.
func set_union(_ *Thread, b *Builtin, args Tuple, kwargs []Tuple) (Value, error) {
var iterable Iterable
if err := UnpackPositionalArgs(b.Name(), args, kwargs, 0, &iterable); err != nil {
return nil, err
if len(kwargs) > 0 {
return nil, nameErr(b, "update does not accept keyword arguments")
andponlin-canva marked this conversation as resolved.
Show resolved Hide resolved
}
iter := iterable.Iterate()
defer iter.Done()
union, err := b.Receiver().(*Set).Union(iter)
if err != nil {
return nil, nameErr(b, err)

resultSet := b.Receiver().(*Set).clone()

for i, arg := range args {
iterable, ok := arg.(Iterable)
if !ok {
return nil, fmt.Errorf("update: argument #%d is not iterable: %s", i+1, arg.Type())
andponlin-canva marked this conversation as resolved.
Show resolved Hide resolved
}
if err := func() error {
iter := iterable.Iterate()
defer iter.Done()
return resultSet.InsertAll(iter)
}(); err != nil {
return nil, err
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah true; I will do that...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
return union, nil

return resultSet, nil
}

// https://github.com/google/starlark-go/blob/master/doc/spec.md#set·update.
Expand Down
3 changes: 3 additions & 0 deletions starlark/testdata/set.star
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,13 @@ assert.eq(list(set("a".elems()).union("b".elems())), ["a", "b"])
assert.eq(list(set("ab".elems()).union("bc".elems())), ["a", "b", "c"])
assert.eq(set().union([]), set())
assert.eq(type(x.union(y)), "set")
assert.eq(list(x.union()), [1, 2, 3])
assert.eq(list(x.union(y)), [1, 2, 3, 4, 5])
assert.eq(list(x.union(y, [6, 7])), [1, 2, 3, 4, 5, 6, 7])
assert.eq(list(x.union([5, 1])), [1, 2, 3, 5])
assert.eq(list(x.union((6, 5, 4))), [1, 2, 3, 6, 5, 4])
assert.fails(lambda : x.union([1, 2, {}]), "unhashable type: dict")
assert.fails(lambda : x.union(1, 2, 3), "argument #1 is not iterable: int")

# set.update (allows any iterable for the right operand)
# The update function will mutate the set so the tests below are
Expand Down