Skip to content
This repository has been archived by the owner on Apr 16, 2024. It is now read-only.

Added: ViewRange Query #14

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

Conversation

frank-dspeed
Copy link

No description provided.

@balassy
Copy link
Owner

balassy commented Aug 22, 2016

@frank-dspeed: Thank you for submitting this pull request. Few questions:

  • Could you please fix the lines that cause the two build errors?
  • What is the reason behind using PascalCase names for the RangeStart and the RangeEnd parameters?
  • I think Line 84 in index.coffee will overwrite the assignment in Line 83, isn't it? Don't we need an else here?

Thanks,
György

@frank-dspeed
Copy link
Author

Will work on that

@frank-dspeed
Copy link
Author

@balassy
Ok Now its merge able

thx

@balassy
Copy link
Owner

balassy commented Aug 23, 2016

@frank-dspeed: Thank you for fixing the build errors, I appreciate it.

I see you kept the RangeStart and the RangeEnd parameters PascalCased, opposed to the usual camelCase convention. What is the reason behind that?

I see you added a new options parameter to the getViewAsync function which is absolutely okay, however I don't see an options function or property on the ViewQuery object in the Couchbase SDK, like you use it in Line 83. Isn't it supposed to be a .custom(options) call?

I recommend avoiding code duplication in Line 81 and 83 with either this:

query = ViewQuery.from(designDocumentName, viewName).key(key).stale(ViewQuery.Update.BEFORE)
if options?
  query = query.custom(options)

or this:

if options?
  query = ViewQuery.from(designDocumentName, viewName).key(key).stale(ViewQuery.Update.BEFORE)
.custom(options)
else
  query = ViewQuery.from(designDocumentName, viewName).key(key).stale(ViewQuery.Update.BEFORE)

Unfortunately I don't have Couchbase at my hand to try any of them. Could you please consider it?

Thanks for contributing to this repo.

György

@frank-dspeed
Copy link
Author

do you know about Our new Couchbase Mock ? its already in the current couchbase SDK

Google Couchbase Mock there is a Near 100% Technical / Protocol Implamented dev Server in the SDK so its like couchbase server light writen in NodeJS

@frank-dspeed
Copy link
Author

@balassy
Applyed your suggestion in form of:
query = ViewQuery.from(designDocumentName, viewName).key(key).stale(ViewQuery.Update.BEFORE)
if options?
query = query.options

even shorter and nothing doubled

@balassy
Copy link
Owner

balassy commented Aug 24, 2016

So it is .options for sure, or should be .custom(options) ?

@dsfields
Copy link

dsfields commented Aug 28, 2016

In an effort to save you some work, there is already a module for Couchbase that promisifies the entirety of the official couchbase module (including ViewRange queries, and all of the new N1QL queries). It is available in NPM as couchbase-promises. The project is thoroughly unit tested, and keeps pace with new features added to the official module.

Just note that couchbase-promises is written in ES2015. If you are using Node.js v0.12 you will need to enable the necessary harmony flags.

@frank-dspeed
Copy link
Author

i know about the complet lib its not my effort to update this i used it only in the beginning and switched to modify the Couchbase ODM aka Ottoman Fork 💃

But some one contributed to my fork and so i whant to contribute that back to the source where it belongs too thats why i care to finish this

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

Successfully merging this pull request may close these issues.

3 participants