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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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.

None yet

3 participants