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.
