And thanks for the review anyway! - benoît
On Tue, Nov 30, 2010 at 1:05 PM, Benoit Chesneau <[email protected]> wrote: > On Tue, Nov 30, 2010 at 12:22 PM, Filipe David Manana > <[email protected]> wrote: >> Hi Benoît, >> >> It's a good idea I think. Good work. >> I have some comments regarding the patch: >> >> 1) Doing a regexp substitution of the view function's code string >> seems like a recipe for disaster to me. >> >> I think the safe and clean way to go here is to create another >> sandbox, with a different 'emit' function, and in >> Couch.compileFunction (utils.js) pass a third and optional argument >> which is the context in which the function is going to be executed. If >> omitted, it uses 'sandbox' as the context, otherwise use that other >> context. Then here: >> >> https://github.com/benoitc/couchdb/commit/651e29e1bb767fb493bf75497623dae89bf9a5ad#L0R34 >> >> You would so something like: >> >> fun = Couch.compileFunction(source, filter_sandbox); >> > > Indeed that's better, will make the change. >> >> 2) Don't forget the indentation level for .js files is 2 spaces (I'm >> seeing a mix of 4 and 2 spaces in share/server/filter.js) >> >> 3) Avoid the unnecessary white-space only changes: >> >> https://github.com/benoitc/couchdb/commit/651e29e1bb767fb493bf75497623dae89bf9a5ad#L2R396 >> https://github.com/benoitc/couchdb/commit/651e29e1bb767fb493bf75497623dae89bf9a5ad#L2R477 >> https://github.com/benoitc/couchdb/commit/651e29e1bb767fb493bf75497623dae89bf9a5ad#L3R255 >> https://github.com/benoitc/couchdb/commit/651e29e1bb767fb493bf75497623dae89bf9a5ad#L4R60 >> https://github.com/benoitc/couchdb/commit/651e29e1bb767fb493bf75497623dae89bf9a5ad#L4R85 >> https://github.com/benoitc/couchdb/commit/651e29e1bb767fb493bf75497623dae89bf9a5ad#L4R247 >> https://github.com/benoitc/couchdb/commit/651e29e1bb767fb493bf75497623dae89bf9a5ad#L4R503 >> https://github.com/benoitc/couchdb/commit/651e29e1bb767fb493bf75497623dae89bf9a5ad#L3R191 > > > about 2 & 3 can we add indent rules on top of our sources from now ? > I think there is something common between vim and emacs for example. > That should solve such errors. I don't want to change my config each > time I'm using a project. Using such rules solves that automatically. > > Ex: > > %% -*- tab-width: 4;erlang-indent-level: 4;indent-tabs-mode: nil -*- > %% ex: ts=4 sw=4 et > > > works on erlang and vim and surely other editors. About the 2 spaces > in js, imo we should go to a 4 spaces indentations, which is a way > more readable and more common. (Mozilla uses that rule.) > >> >> 4) This?LOG_INFO line is there for your debugging purposes I think >> (should be removed): >> >> https://github.com/benoitc/couchdb/commit/651e29e1bb767fb493bf75497623dae89bf9a5ad#L3R168 >> >> > yup. >
