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

add is_crdt convenience fn #1081

Conversation

zeeshanlakhani
Copy link
Contributor

Convenience fun related to @seancribbs's comment on basho/yokozuna#452 (diff). If there's a better name for this fun, I'll update it accordingly :).

-spec is_crdt(riak_object:riak_object()) -> boolean().
is_crdt(RObj) ->
Bucket = riak_object:bucket(RObj),
case riak_core_bucket:get_bucket(Bucket) of
Copy link
Member

Choose a reason for hiding this comment

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

Why the case statement and not just:

BProps = riak_core_bucket:get_bucket(Bucket),

?

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 kinda stole most of this from https://github.com/basho/riak_kv/blob/feature/zl/helper-fn-check-if-object-bucket-props-supports-crdt/src/riak_kv_crdt.erl#L103 haha, which probably has it just for the guard. But, you're right, the case is unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at other spots, e.g.

-spec consistent_object(binary() | {binary(),binary()}) -> true | false | {error,_}.
, I decided to keep the case, but actually have an error case if BProps is not a list for some reason.

@russelldb
Copy link
Member

Looks good to me, although a unit test wouldn't hurt.

@zeeshanlakhani
Copy link
Contributor Author

Yeah... I'm basically testing it for the use-case here, https://github.com/basho/yokozuna/pull/452/files#diff-a765b250cdc4d78ac1eef6cfddbbb280R508, but I'll add a unit test.

@russelldb
Copy link
Member

Thanks, I think a unit test here is needed. Just something simple, meck out riak_core_bucket and return buckets for all the datatypes, some datatype=nonsense, and some no datatype is exhaustive.

@zeeshanlakhani zeeshanlakhani force-pushed the feature/zl/helper-fn-check-if-object-bucket-props-supports-crdt branch from f00820d to 6638df3 Compare February 15, 2015 11:50
@zeeshanlakhani zeeshanlakhani force-pushed the feature/zl/helper-fn-check-if-object-bucket-props-supports-crdt branch from 6638df3 to 9f17bcc Compare February 15, 2015 11:51
@seancribbs
Copy link
Contributor

👍 9f17bcc

borshop added a commit that referenced this pull request Feb 19, 2015
…ect-bucket-props-supports-crdt

add is_crdt convenience fn

Reviewed-by: seancribbs
@zeeshanlakhani
Copy link
Contributor Author

@borshop merge

@borshop borshop merged commit 9f17bcc into develop Feb 19, 2015
@seancribbs seancribbs deleted the feature/zl/helper-fn-check-if-object-bucket-props-supports-crdt branch February 19, 2015 20:40
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

Successfully merging this pull request may close these issues.

4 participants