google / starlark-go

Starlark in Go: the Starlark configuration language, implemented in Go
BSD 3-Clause "New" or "Revised" License
2.31k stars 210 forks source link

Additional methods for Set type #497

Closed SamWheating closed 1 year ago

SamWheating commented 1 year ago

Re: https://github.com/google/starlark-go/issues/492

Added set.add(x), set.pop(), set.remove(x), set.discard(x), set.clear(), as well as updates to the Spec and tests.

Demo:

>>> a = set([1,2,3])
>>> a.add(4)
>>> a
set([1, 2, 3, 4])
>>> a.remove(1)
>>> a
set([2, 3, 4])
>>> a.remove(1)
Traceback (most recent call last):
  <stdin>:1:9: in <expr>
Error in remove: remove: missing key
>>> a.discard(2)
>>> a
set([3, 4])
>>> a.discard(2)
>>> a.pop()
3
>>> a
set([4])
>>> a.pop()
4
>>> a.pop()
Traceback (most recent call last):
  <stdin>:1:6: in <expr>
Error in pop: pop: empty set
>>> a.add(1)
>>> a.add(2)
>>> a
set([1, 2])
>>> a.clear()
>>> a
set([])
SamWheating commented 1 year ago

Thanks for the super thorough review - I think I have addressed all of your feedback now and I left a few discussions open where I need some additional confirmation.

SamWheating commented 1 year ago

Thanks for the review, got everything addressed aside from a few open discussions.

While you're here - how would you feel about adding built-in set parsing and set comprehension? I'd be interested in working on the implementation but also think this might be too much divergence from the official spec.

adonovan commented 1 year ago

how would you feel about adding built-in set parsing and set comprehension? I'd be interested in working on the implementation but also think this might be too much divergence from the official spec.

I would rather not implement those since they change the dialect accepted by the parser. If you can persuade the Bazel folks to accept set as a standard feature, then these expressions would be appropriate, but I don't think they feel much need for it.

Thanks again for contributing this, and for your patience during the review.