Add support for Mango queries - #20
Conversation
| 'useCache', | ||
| 'listDatabases', 'createDatabase', 'dropDatabase', | ||
| 'insert', 'update', 'del', 'get', | ||
| 'insert', 'update', 'del', 'get', 'mango', |
There was a problem hiding this comment.
I would say that there's not enough testing for this new feature. Could you please add some more tests? And I also think that this mango() method should throw if couchdb version is less than 2. What do you think about this?
There was a problem hiding this comment.
I think it should just forward whatever the database returns for the request. So the promise returned will reject, yes?
I didn't add tests because I was not sure what version you were running the tests against, but shouldn't be a problem if your CI is using 2.0. Is it?
I've been using this in production for a couple of weeks and it's really nice.
I also added a method for _changes and that's been really sweet, but required a bit of plumbing to implement the streaming. Was waiting on this one before proceeding with that PR.
There was a problem hiding this comment.
I think it should just forward whatever the database returns for the request. So the promise returned will reject, yes?
In this case behaviour for CouchDB@1 and CouchDB@2 will be different (couchdb@2 will return smth and couchdb@1 will reject with 404), right? I think it would be better if node-couchdb returns some kind of special error (EOLDSERVER for instance) in this case instead of letting user guess what happened based on 404 server response status code.
There was a problem hiding this comment.
Either way, because both Couch versions have well documented the response, maybe it's best to not try to wrap one error in another. This would DRY up the documentation, and be more maintainable.
Another approach would be to maintain a list of v2 functions and do as you suggest, throw a version error.
Let me know your thoughts and I will update the PR.
There was a problem hiding this comment.
Your current PR is better than what I suggested. No changes are needed here for tests.
There was a problem hiding this comment.
we can add version checking in another PR. OK?
| * - resolved with {data, headers, status} object | ||
| * - rejected with `request` original error | ||
| * | ||
| * @param {String} dbName database name |
| { | ||
| "name": "node-couchdb", | ||
| "version": "1.1.0", | ||
| "version": "1.1.1", |
| 'useCache', | ||
| 'listDatabases', 'createDatabase', 'dropDatabase', | ||
| 'insert', 'update', 'del', 'get', | ||
| 'insert', 'update', 'del', 'get', 'mango', |
There was a problem hiding this comment.
Your current PR is better than what I suggested. No changes are needed here for tests.
|
@Flamenco could you please add the changes I ask for? |
|
I should have something in the next day or two. I added a bunch of changes for streaming the _changes endpoint and they are in my same branch. I need to refactor them into a separate PR. |
62ac50a to
1baea57
Compare
|
OK, I updated comment and will add my other PRs in a separate feature branch. |
| 'useCache', | ||
| 'listDatabases', 'createDatabase', 'dropDatabase', | ||
| 'insert', 'update', 'del', 'get', | ||
| 'insert', 'update', 'del', 'get', 'mango', |
There was a problem hiding this comment.
Did you want me to remove 'insert', 'update', 'del', 'get', 'mango', <----- ?
Not sure if:
Your current PR is better than what I suggested. No changes are needed here for tests
applied to that line or just in general....
2 participants
No description provided.