@helper and optional parameters #41

Closed
Radagaisus opened this Issue Sep 4, 2012 · 3 comments

Comments

Projects
None yet
2 participants
@Radagaisus

This just bit me, and I'm wondering why is this necessary:

  apply_helpers = (ctx) ->
    for name, helper of helpers
      do (name, helper) ->
        if typeof helper is 'function'
          ctx[name] = (args...) ->
            args.push ctx  # ???
            helper.apply ctx, args
        else
          ctx[name] = helper
    ctx

Why do we need to add the context to the arguments? It's already available as, you know, the context.

It's very difficult to use optional arguments with this, and it's not documented.

@shimaore

This comment has been minimized.

Show comment
Hide comment
@shimaore

shimaore Sep 4, 2012

Member

That's actually a good question already pointed out in mauricemach/zappa#99 and I honestly have no proper answer, although I suspect this might be have been related to setting databag to this, in which case the context should be the databag -- but that code never made it into helpers, and the original commit that added this didn't have a justification for it so I don't know what the scenario would be.

My proposal is to:

  1. remove the extraneous argument from helpers;
  2. get rid of databag == "this" to ensure that the context is always @.

(Since I was the one who asked for databag == "this" but never used it, I suspect there isn't much usage around.)

We can keep databag (or introduce a new setting, maybe params?) as a boolean, to replace databag == "param". (This indicates that the databag should be passed as argument to handlers. There's a performance penalty in doing so, so I don't want to make it the default though.)

Member

shimaore commented Sep 4, 2012

That's actually a good question already pointed out in mauricemach/zappa#99 and I honestly have no proper answer, although I suspect this might be have been related to setting databag to this, in which case the context should be the databag -- but that code never made it into helpers, and the original commit that added this didn't have a justification for it so I don't know what the scenario would be.

My proposal is to:

  1. remove the extraneous argument from helpers;
  2. get rid of databag == "this" to ensure that the context is always @.

(Since I was the one who asked for databag == "this" but never used it, I suspect there isn't much usage around.)

We can keep databag (or introduce a new setting, maybe params?) as a boolean, to replace databag == "param". (This indicates that the databag should be passed as argument to handlers. There's a performance penalty in doing so, so I don't want to make it the default though.)

shimaore added a commit that referenced this issue Sep 4, 2012

Fix for #41
(and mauricemach/zappa#99 incidentally)

shimaore added a commit that referenced this issue Sep 4, 2012

Removed extraneous argument to callback functions.
Callbacks are now all called without any parameters. See #41

shimaore added a commit that referenced this issue Sep 4, 2012

Modified the databag option.
Esp. removed the `this` version of it. See #41
@shimaore

This comment has been minimized.

Show comment
Hide comment
@shimaore

shimaore Sep 4, 2012

Member

OK, here it is. This is pretty extensive, and hasn't been through tests yet.
Also I need to double-check the reference and crashcourse documents.

Member

shimaore commented Sep 4, 2012

OK, here it is. This is pretty extensive, and hasn't been through tests yet.
Also I need to double-check the reference and crashcourse documents.

@Radagaisus

This comment has been minimized.

Show comment
Hide comment
@Radagaisus

Radagaisus Sep 4, 2012

Looks good. I actually have no idea what the databag is even used for. It's only mentioned very briefly in the docs and I doubt anyone actually use it.

Looks good. I actually have no idea what the databag is even used for. It's only mentioned very briefly in the docs and I doubt anyone actually use it.

@shimaore shimaore closed this Aug 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment