couchdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Russell Branca <chewbra...@gmail.com>
Subject Re: couchdb pull request: Many fixes to $.couch
Date Tue, 23 Oct 2012 17:37:32 GMT
I took a quick peek and the code looks like the expected changes to
switch jquery.couch.js to use jquery deferreds, which in my opinion,
is a good addition. I haven't run the test suite with it, but assuming
it doesn't break the existing callback based functionality, it would
be an ubobtrusive and good feature to add.

For the changes to the urlPrefix functionality, I've got a couple
questions about the statement: "removed the $.couch.urlPrefix, which
wasn't working correctly anyway. since it wasn't working correctly i
assume nobody is using it." What part of it isn't working? Currently
urlPrefix allows you to specify a subdomain url for a database server
as expected, and I've used it on many occasions.

What the changes to urlPrefix in this pull request do appear to
accomplish, is that by delaying the insertion of the base url from the
initial creation of the db object [1] into the methods themselves ([2]
for instance), you will then be able to set the url after you have
created a db object. For instance, if you do:


$.couch.urlPrefix = "http://me.mydb.com"
var db = $.couch.db("some_db")
$.couch.urlPrefix = "http://otherdb.com"


In the current jquery.couch.js, urlPrefix will be hardcoded into the
db variable, and further updates to $.couch.urlPrefix would not be
brought in, whereas the patch would allow that to happen, which is
arguablly a useful feature. My guess is that the "broken"
functionality he was seeing was trying to set $.couch.urlPrefix after
already having created a db object, which will not work.

In my opinon, the name change from urlPrefix to url is an unneeded API
change that will require code updates as people are currently using
urlPrefix. The switch to deferreds should be unobtrusive, and keeping
urlPrefix as the name will also keep the patches unobtrusive.


Couple minor nits:

The ajax deferred should be returned in [3].
Need to add $.couch.url in [4].


Overall looks like a good patch. I'm a big fan of jquery deferreds for
ajax and they would be a welcome addition to jquery.couch.js, assuming
that the test suite passes with this and the existing callbacks are
unaffected.



-Russell


[1] https://github.com/apache/couchdb/pull/34/files#L0L284
[2] https://github.com/apache/couchdb/pull/34/files#L0R310
[3] https://github.com/apache/couchdb/pull/34/files#L0L316
[4] https://github.com/apache/couchdb/pull/34/files#L0L340

On Tue, Oct 23, 2012 at 6:24 AM, Noah Slater <nslater@apache.org> wrote:
> Can someone take a look at this?
>
> On 16 October 2012 18:23, boxxxie <git@git.apache.org> wrote:
>
>> https://github.com/apache/couchdb/pull/34
>
>
>
>
> --
> NS

Mime
View raw message