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.

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

Prevent riakc_set:new/2 when the context is undefined (or patch to_op) #152

Closed
macintux opened this issue Jan 19, 2014 · 10 comments
Closed

Comments

@macintux
Copy link
Contributor

At a glance, creating a set from scratch with riakc_set:new(List, undefined) seemed like a reasonable approach, but riakc_set:to_op/1 doesn't anticipate this case, thus making any set created and stored only contain any elements added after the new/2 invocation.

I'd suggest returning an error when context is undefined, or...

It's also possible to make to_op smart enough to handle this scenario; I haven't conducted any more than minimal tests, so it's possible that the combination of new/2 plus the smarter to_op breaks a half dozen other things.

https://gist.github.com/macintux/1f34d0443fdf029161c6

@macintux
Copy link
Contributor Author

As an argument for allowing new/2 to be invoked for entirely new sets, here's what it takes to create a set from a list of values today, unless I'm missing something:

     lists:foldl(fun(X, Set) -> riakc_set:add_element(X, Set) end,
                 riakc_set:new(), ListOfValues)}.

Perhaps that's more of an argument for creating riakc_set:add_list than for allowing new/2 to be used this way.

@seancribbs
Copy link
Contributor

@macintux Yes, the new/2 is only intended to be invoked from loading a datatype from the server. However, I don't see any reason an add_all, from_list, or union function couldn't be added.

The main difficulty is that these datatypes are only shadows of the ones on the Riak cluster, so their goal on the client-side is to encapsulate mutations, not to act like regular Erlang data structures.

@seancribbs
Copy link
Contributor

Another option might be to rename new/2 to load/2. At least then it would be obvious that it isn't intended for users to create.

@macintux
Copy link
Contributor Author

What's the intended use case for new/2 besides the EQC test?

@seancribbs
Copy link
Contributor

Yes, the new/2 is only intended to be invoked from loading a datatype from the server.

^^

@macintux
Copy link
Contributor Author

Ok, so my reading skills aren't top notch. I'm still a bit confused as to when it would be used, though. riakc_pb_socket:fetch_type returns a populated set record. The only reason to use it that jumps to mind is to overwrite an existing set with an entirely new set of values.

@macintux
Copy link
Contributor Author

Oh

@macintux
Copy link
Contributor Author

Yeah, I'm a little slow. How about load_from_server?

@seancribbs
Copy link
Contributor

shrug At the very least, documentation is in order.

@macintux
Copy link
Contributor Author

Documentation for data types is available via docs.basho.com and the README. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants