Skip to content

Return {ok,CAS::integer()} instead of ok for all store operations.#86

Open
vincentdephily wants to merge 2 commits intochitika:masterfrom
vincentdephily:vdp_ok_cas
Open

Return {ok,CAS::integer()} instead of ok for all store operations.#86
vincentdephily wants to merge 2 commits intochitika:masterfrom
vincentdephily:vdp_ok_cas

Conversation

@vincentdephily
Copy link
Contributor

Return {ok,CAS::integer()} instead of ok for all store operations.

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().

Also includes spec fixes that should be uncontroversial.

This is the long-delayed re-posting of PR #62 . Priorities had shifted at work and I forgot to finish this up (we were using my fork in the meantime.

As mentioned in the old PR, this commit changes the API, so you'll want to bump the version to 1.1.x to attract people's attention to the release notes. Given that n1ql support has also landed, it looks like a goo opportunity.

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().
@vasumahesh1
Copy link

Any update on this? Currently using my fork, would wanna switch to the main repo.

@vincentdephily
Copy link
Contributor Author

Last time I posted this it was well received, so I think it's just waiting for somebody with commit rights to re-check that it looks ok, and click merge. More reports that this feature is wanted and used can only help :)

@wcummings
Copy link
Contributor

@vincentdephily I'll take a look at this in the next day or two as time allows, and then @fauxsoup can merge it, or at the very least I'll merge it into my fork

@wcummings
Copy link
Contributor

Gotta update some tests. There's also the question of breaking API compatibility, people have complained about it breaking before, might be worth making some new functions or module. I've merged your changes into my fork, I'll make these tweaks and open a new PR.

@wcummings wcummings mentioned this pull request Dec 6, 2016
@wcummings
Copy link
Contributor

@vasumahesh1 There's a release here which includes a change to return CAS values for store operations, it's a breaking change. https://github.com/wcummings/cberl

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.

3 participants