Ensure immutability of sets, maps & lists #39
Conversation
There was a problem hiding this comment.
Good catch. Not enough tests on my part, sorry.
I'd do this a little differently though, to avoid the creation of a new immutable.Map / immutable.Set for each item. I think initialising the new Sets with s.Items()... could be quite nice in this case.
For Map.SetMany I'd do something similar to the existing Map.set(), but with a for loop. I could offer another solution in a day or two for the maps in particular. I could also add more tests.
FWIW I think it's probably fine to merge this PR for correctness, and then merge my changes later.
There was a problem hiding this comment.
This is the best way, for the time being. Sorry I didn't realise Maps were so stateful when I PR'd the varargs changes.
Aside: I wonder if we should revert Set.Set() to being non-varargs, and to remove Map.SetMany(). They are convenient, but IMO unexpectedly greedy. MapOf() and NewSet() both support efficient putting of multiple items, and I figure that's enough.
|
No worries. 🙂 I agree with the API changes. If the user wants those conveniences, they're simple enough to implement on the side. |
Up to you, whatever you prefer. (BTW I don't have any authority on this repo, I'm just a random) |
BarrensZeppelin
changed the title
The implementation behind the API is not more efficient than manually looping over a data structure and inserting elements one-by-one.
Similar reason for the API change as the previous commit.
The closure passed to t.Run captured the loop variable which would always be equal to `*randomN` once the tests started running. This meant that all of the runs used the same random seed.
|
The Since these updated functions have already been published in v0.4.2, it might be a good idea to mark that version of the library as retracted? @benbjohnson |
|
@BarrensZeppelin Sorry for the delay in reviewing this. Looks good. Thanks for catching this. I'll retract v0.4.2. |
As I wrote in #32 (comment) the current implementation of sets is broken, as well as the
SetManyimplementations.It is not safe to perform updates with
mutable=trueafter callingclone().