Sorry for the delay Benoît. Comments:
1) At https://github.com/benoitc/couchdb/compare/master...view_changes#L1R40 you're also changing the emit of the regular sandbpox (don't forget, in the previous statement you're copying a reference, not a value): E.g.: $ ./couchjs - c1 = evalcx(''); c1.foo = "bar"; c2 = c1; c2.foo = "hello world"; print("c1.foo: " + c1.foo); print("c2.foo: " + c2.foo); <EOF> c1.foo: hello world c2.foo: hello world 2) couch_changes:filter_view/4 has an unused parameter in both clauses (Req) - it's better to remove it? 3) The error message at https://github.com/benoitc/couchdb/compare/master...view_changes#L1R40 uses single quotes. Everywhere else we use back ticks. 4) Perhaps here: https://github.com/benoitc/couchdb/compare/master...view_changes#L4R235 the name of the command sent to the view server should be something like 'view_filter' instead of 'views', which to me is a bit misleading As for 1), 2) and 3) I committed into my own branch: https://github.com/fdmanana/couchdb/commit/1e5498abcdc005c24961685e37359809bf3fedad good work regards, On Mon, Dec 6, 2010 at 8:36 AM, Benoit Chesneau <[email protected]> wrote: > bump. > > On Tuesday, November 30, 2010, Benoit Chesneau <[email protected]> wrote: >> On Tue, Nov 30, 2010 at 4:44 PM, Filipe David Manana >> <[email protected]> wrote: >>>> >>>> Here is an updated patch. It now use a filter sandbox instead of >>>> patching the function and it fixes whitespaces. >>> >>> Why a separate function to init the filter_sandbox? It can be done at >>> the end of the existing init_sandbox() function (perhaps renaming it >>> to init_sandboxes). >>> >>> I would rename Filter.filter_view to Filter.view_filter or to >>> Filter.map_filter (doing the same in >>> src/couchdb/couch_query_servers.erl for naming consistency). >>> >>> 4) is still there (the debugging ?LOG_INFO line). >>> >>> Also, this patch should only contain the changes necessary to >>> implement the new feature. While all the whitespace and indentation >>> fixes (to comply with the wiki rules) are a welcome plus, they should >>> come in a separate patch imo. >>> >> Well I used filter_view for consistency, since we use filter_docs >> already, I don't think it need to be changed at this point. I've >> removed the log stuff. >> >> Are we OK for such feature ? If yes, I will commit it. >> >> - benoit >> > -- Filipe David Manana, [email protected], [email protected] "Reasonable men adapt themselves to the world. Unreasonable men adapt the world to themselves. That's why all progress depends on unreasonable men."
