search

djc / couchdb-python

Python library for working with CouchDB
Other
202 stars 86 forks source link

fix changes feed for couchbase sync gateway #289

Closed hermitdemschoenenleben closed 8 years ago

djc commented 8 years ago

Hey, thanks for looking into fixing this!

First of all, obviously this will need to pass Travis' tests to be accepted.

Second of all, I don't think we want to have both iterchunks() and iterdocs(). Can we just fix up iterchunks() to do the right thing? Can you paste some demonstrations of ways in which Couchbase breaks the current implementation? I.e., could we still split by line (but not by chunk) and get a single doc, or do they split their updates over response content lines, as well?

hermitdemschoenenleben commented 8 years ago

Hello,

I introduced iterdocs() because I needed a quick fix and didn't want to break something. I now looked into it again and thought that I found an easier solution, however it breaks couchdb compatibility... I'll try again...

hermitdemschoenenleben commented 8 years ago

ok, now it should work with couchdb as well as couchbase sync. How can I run Travis again for the last commit?

djc commented 8 years ago

You should probably rebase on top of master, then push to your PR branch again (with --force).

hermitdemschoenenleben commented 8 years ago

ok, now it should work. Note however that I changed the test_double_iteration_over_same_response_body test such that the test data ends with a \n, because couchdb states that JSON messages are terminated by a newline character (http://docs.couchdb.org/en/1.6.1/query-server/protocol.html)

djc commented 8 years ago

Fails on Python 3. Do you think you can fix that?

hermitdemschoenenleben commented 8 years ago

Hmm, the error at travis using python 3 is "No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself", I'm not sure whether this is really caused by my small change... (I don't have the time to test it using python3 at the moment, but I will look into it...)

djc commented 8 years ago

Well, on my server I can reproduce that the Python 2 tests pass with your changes, whereas with Python 3 the tests hang. So this definitely still needs some work before it can be merged.

hermitdemschoenenleben commented 8 years ago

ok, now it should work with python 3, too.

djc commented 8 years ago

Would you mind giving me a real email address to include in your commit? It looks like you had your git commit identity email set to "ben@localhost.localdomain" when committing this. Thanks!

hermitdemschoenenleben commented 8 years ago

damn, the tests still fail... I'll look into this again once I have a little bit more time (in about a month)

hermitdemschoenenleben commented 8 years ago

ok, found the time to finally fix this, now the tests pass and it works with couchbase sync gateway, too :) (I also fixed my git configuration to use my right email adress)

hermitdemschoenenleben commented 8 years ago

is there anything still missing?

djc commented 8 years ago

No, I've had this sit open in a tab waiting for some time to review. As it is, it feels like a lot of complexity to support a backend that I imagine is rare among CouchDB-Python users, so I want to take some time and see if it can be simplified (in terms of lines of code, say). Sorry that I have not yet gotten to it, I am aware that you have invested quite some time in this PR.

djc commented 8 years ago

I (finally) squashed your changes into ae055cec3c126188144e70f9e55374890af3f3f7, and then did some followup cleanups in c88ba9e1aa3c35ccf925de0de30a3fffd765c0ff, a5cd8a21df3368faee39465c05822fd821c2ce88 and 88fcad9a77c732a86b1e35d66af32953744d555e. Thanks for sticking with it, and sorry for the delay!

hermitdemschoenenleben commented 8 years ago

Thank you for merging and don't worry about the delay!

djc commented 8 years ago

Also, I just released the 1.1 release that includes this.