-
Notifications
You must be signed in to change notification settings - Fork 65
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
Return CAS value #87
Return CAS value #87
Conversation
This saves a roundtrip when you set/add an item that you'll want to modify later. This is a backward-incompatible change, but it seems important enough and seems to be a coding oversight as most of the work was already done (but thrown away) in store_callback().
I notice https://github.com/orgs/chitika/people is empty now, might be time to start using my repo as the trunk 😂 |
@wcummings in the original PR (#62) you suggested merging the optional arguments together into a map (or proplist or record or...), I think that would work well and we can do this fairly elegantly: % @deprecated
store(PoolPid, Op, Key, Value, TranscoderOpts, Exp, Cas) ->
execute(PoolPid, {store, Op, Key, Value, #{trancoder => TranscoderOpts, exp => Exp, cas => Cas}).
% New default function
store(PoolPid, Op, Key, Value, Opts) ->
execute(PoolPid, {store, Op, Key, Value, Opts}).
% Simple version
store(PoolPid, Op, Key, Value) ->
execute(PoolPid, {store, Op, Key, Value, #{}}).
% Some functions require some where trickery but shouldn't need a name change
append(PoolPid, Cas, Key, Value) when is_integer(Cas) ->
store(PoolPid, append, Key, Value, {cas => Cas});
append(PoolPid, Key, Value, Opts) ->
store(PoolPid, append, Key, Value, Opts).
append(PoolPid, Key, Value) ->
store(PoolPid, append, Key, Value, #{}). From there on, the API should be a bit more future-proof and it'll be easy to add a |
@vincentdephily 👍 I can make this happen |
@vincentdephily So i think this basically works, but the problem I've noticed is situations like this: add(PoolPid, Key, Value) ->
add(PoolPid, Key, Value, []).
add(PoolPid, Key, Value, Opts) when is_list(Opts) ->
store(PoolPid, add, Key, Value, Opts);
%% @equiv add(PoolPid, Key, Exp, Value, standard)
%% @deprecated
add(PoolPid, Key, Exp, Value) ->
store(PoolPid, add, Key, Value, [{exp, Exp}]). If the key is a string (list of chars) the guard won't save us. Similarly, a value can be an integer, just like an expiration. I think the best solution is to simply remove the newly deprecated functions, but I'm open to suggestions. Since a value can be basically anything it's pretty hard to resolve this "clash". Even a tagged tuple could potentially be a valid value, depending on the transcoder being used. The options I see are:
I'll push my changes shortly. |
OK after some deliberation I'm going with the original change, it breaks compatibility but it seems that was unavoidable and this is just a lot simpler and probably an easier change for people. It's in my repo w/ a tagged release. https://github.com/wcummings/cberl |
Tests pass, dialyzer is happy
This is a breaking change, but I agree w/ @vincentdephily that it is important enough. Because it affects so many functions, the alternatives would severely muddy the interface (for ex we could create a new set of store functions in a
cberl_v2
module, or a new set of functions w/ a different arity, essential a "return a cas" flag). This should be accompanied by a version bump.#86
cc @vasumahesh1
P.S.
""
doesnt give you the default bucket anymore, this includes a commit to pass"default"
instead