Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding iterate helper #45

Closed
wants to merge 4 commits into from
Closed

Conversation

rashad612
Copy link

Adding iterate helper, to iterate over object and get any key|value storage blindly.
rashad612/dustjs-helpers@9039130

@rragan
Copy link
Contributor

rragan commented Jun 4, 2013

As you no doubt know, this has been discussed off and on for a long time. As I see it the issues are:

  • Users are likely to fall into the trap of expecting output values to be consistent across browsers. There is no guarantee of this and I believe Chrome is different from other browsers. Just yesterday I had someone asking for this to generate a dropdown menu which usually has a particular ordering desired.
  • Adding this to the standard dust helpers adds cost for all dust users of helpers. Granted the case seems very basic but, in practice, the number of cases where you don't care about order is fairly small. Proposals to provide a place to contribute such helpers, ala Bower components, might be a better way to go.

Tactical concerns:

  • You need tests with your pull request
  • By convention only, the default param name is "key". There are exceptions but the more variations created, the more memory-load on the user trying to recall what the param is named.

@rashad612
Copy link
Author

I think we need something like 'dustjs-extras', so that we can keep the core and default helpers in fair size.

params = params || {};
var obj = params['on'] || context.current();
for (var k in obj) {
chunk = chunk.render(bodies.block, context.push({key: k, value: obj[k]}));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this would need to be wrapped in a if (obj.hasOwnProperty(k)) check.

@vybs
Copy link
Contributor

vybs commented Jun 5, 2013

I am reading the comment from @akdubya and he says it very well. I miissed his explanation earlier.

@rragan
Copy link
Contributor

rragan commented Jun 5, 2013

Here is the link where you can see akdubya's comment. He suggested a contextual helper rather than a built in solution. Here is the ref to see his comment: akdubya/dustjs#9

@brikis98
Copy link

brikis98 commented Jun 6, 2013

@rragan I read through the thread, and to be honest, I don't see what is the counter argument to supporting this. The response was basically "use a helper".

This would make sense if it was something extremely esoteric, but a number of people requested it and it comes up again and again. It's a tiny bit of code. It makes the looping support consistent within dust and consistent with every other programming language.

The only vaguely tangible push-back has been that the loop will have a non-deterministic order. But that's always true for looping over objects/hashes, and yet people do it all the same.

@brikis98
Copy link

brikis98 commented Jun 6, 2013

BTW, one change worth considering is if we can support looping over objects and lists with the same # syntax. Here are two examples:

JSON:

{
  "obj": { 
    "a": "b",
    "c": "d",
    "e": "f"
  },
  "list": [
    "a",
    "b",
    "c"
  ]
}

Dust:

Looping over a list:
{#list key="key" value="value"}
  {key}: {value}
{/list}

Looping over an object:
{#obj key="key" value="value"}
  {key}: {value}
{/obj}

Output:

Looping over a list:
0: a
1: b
2: c

Looping over an object:
a: b
c: d
e: f

@rashad612
Copy link
Author

@brikis98 It really does make sense to be part of the context.

@sclatter
Copy link

sclatter commented Jun 6, 2013

I agree having an object iterator in the context the same as how it works for lists makes sense. One case we should consider in the implementation and testing is nested iterations. For example:

{
  "obj": { 
    "a": "b",
    "c": "d",
    "list": ["a", "b", "c"]
  }
}

And then what something like this would output:

{#obj key="key" value="value"}
  {key}: {value}
  {#list key="key" value="value"}
    {key}: {value}
  {/list}
{/obj}

@smfoote
Copy link
Contributor

smfoote commented Jun 6, 2013

I am not entirely against having an iterator helper, but I strongly
disagree with including it as part of the "section." A section is a way to
move into different levels of the context. As an added benefit, if the
level of the context you enter happens to be an array, you loop through the
array. However, the primary purpose of the section is moving, not looping.

On Thu, Jun 6, 2013 at 10:08 AM, sclatter notifications@github.com wrote:

I agree having an object iterator in the context the same as how it works
for lists makes sense. One case we should consider in the implementation
and testing is nested iterations. For example:

{
"obj": {
"a": "b",
"c": "d",
"list": ["a", "b", "c"]
}
}

And then what something like this would output:

{#obj key="key" value="value"}
{key}: {value}
{#list key="key" value="value"}
{key}: {value}
{/list}
{/obj}


Reply to this email directly or view it on GitHubhttps://github.com//pull/45#issuecomment-19059657
.

@sclatter
Copy link

sclatter commented Jun 6, 2013

Should we then make a explicit split between section/context and looping? We could leave the existing behavior, but then introduce a looping/iterating operator that works either on arrays or objects. 


From: Steven notifications@github.com
To: linkedin/dustjs-helpers dustjs-helpers@noreply.github.com
Cc: sclatter sarah@secadvertising.com
Sent: Thursday, June 6, 2013 10:21 AM
Subject: Re: [dustjs-helpers] Adding iterate helper (#45)

I am not entirely against having an iterator helper, but I strongly
disagree with including it as part of the "section." A section is a way to
move into different levels of the context. As an added benefit, if the
level of the context you enter happens to be an array, you loop through the
array. However, the primary purpose of the section is moving, not looping.

On Thu, Jun 6, 2013 at 10:08 AM, sclatter notifications@github.com wrote:

I agree having an object iterator in the context the same as how it works
for lists makes sense. One case we should consider in the implementation
and testing is nested iterations. For example:

{
"obj": {
"a": "b",
"c": "d",
"list": ["a", "b", "c"]
}
}

And then what something like this would output:

{#obj key="key" value="value"}
{key}: {value}
{#list key="key" value="value"}
{key}: {value}
{/list}
{/obj}


Reply to this email directly or view it on GitHubhttps://github.com//pull/45#issuecomment-19059657
.


Reply to this email directly or view it on GitHub.

@smfoote
Copy link
Contributor

smfoote commented Jun 6, 2013

I think the object-iterating use case is different enough that it should be
its own helper. Let's leave the current behavior as is.

On Thu, Jun 6, 2013 at 10:37 AM, sclatter notifications@github.com wrote:

Should we then make a explicit split between section/context and looping?
We could leave the existing behavior, but then introduce a
looping/iterating operator that works either on arrays or objects.


From: Steven notifications@github.com
To: linkedin/dustjs-helpers dustjs-helpers@noreply.github.com
Cc: sclatter sarah@secadvertising.com
Sent: Thursday, June 6, 2013 10:21 AM
Subject: Re: [dustjs-helpers] Adding iterate helper (#45)

I am not entirely against having an iterator helper, but I strongly
disagree with including it as part of the "section." A section is a way to
move into different levels of the context. As an added benefit, if the
level of the context you enter happens to be an array, you loop through
the
array. However, the primary purpose of the section is moving, not looping.

On Thu, Jun 6, 2013 at 10:08 AM, sclatter notifications@github.com
wrote:

I agree having an object iterator in the context the same as how it
works
for lists makes sense. One case we should consider in the implementation
and testing is nested iterations. For example:

{
"obj": {
"a": "b",
"c": "d",
"list": ["a", "b", "c"]
}
}

And then what something like this would output:

{#obj key="key" value="value"}
{key}: {value}
{#list key="key" value="value"}
{key}: {value}
{/list}
{/obj}


Reply to this email directly or view it on GitHub<
https://github.com/linkedin/dustjs-helpers/pull/45#issuecomment-19059657>
.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHubhttps://github.com//pull/45#issuecomment-19061505
.

@brikis98
Copy link

brikis98 commented Jun 6, 2013

@smfoote I'm not sure I understand your concern. While looping over an object, aren't you entering different levels into a context in exactly the same way?

@rragan
Copy link
Contributor

rragan commented Jun 6, 2013

Objects and arrays are different in JavaScript and have different ways to iterate over them. The user should know which they have and express the iteration differently in dust. A separate helper takes care of that.

Otherwise we have to start thinking about edge cases like what is the value of $idx and $len when an iteration is over an object instead of an array.

@brikis98
Copy link

brikis98 commented Jun 6, 2013

Except libraries like jQuery and CoffeeScript provide a forEach or for comprehension that makes the looping more consistent, since that tends to be a much better user experience than the native JavaScript one.

@rashad612
Copy link
Author

Talking about idx and len I think we need a good iterattion helper to deal with objects/arrays in one standard way.

@rragan
Copy link
Contributor

rragan commented Jun 6, 2013

Maybe some checking is in order in terms of the tag. If I do something silly like {@iterate on=xxx/} things blow up in mysterious ways because there is no body. This should either be a meaningful error or the helper should quietly do nothing -- which kind of makes sense.

@rragan
Copy link
Contributor

rragan commented Jun 6, 2013

The iteration helper above works on objects and arrays already and it works on strings. It all derives from how JavaScript for/in loops work on objects: http://javascriptweblog.wordpress.com/2011/01/04/exploring-javascript-for-in-loops/

{@iterate on=arr"} {key}:{value},{/iterate} when arr is [1,2,3,4] produces
0:1,1:2,2:3,3:4,
.

@rragan
Copy link
Contributor

rragan commented Jun 12, 2013

Perhaps we should consider a more enhanced iterate helper with an option to process they keys in sorted order. This would eliminate any browser dependence. I guess taking them in native order with no sort ought to be allowed but with appropriate educational documentation/warnings as there are legitimate use cases.

@rragan
Copy link
Contributor

rragan commented Jun 13, 2013

Since I just had to solve arbitrary nested iteration of an object (assuming an iter helper), I thought I'd share it here in case anyone needs it:

First a not completely general solution. It shows how to descend levels in a structured object provided you know the keys which represents your sub-object of interest. Realistically, you may want to format different parts of your object differently so separate partials for various sub-parts seems plausible.

{@iter key=obj}
{@if cond="'{key}' == 'obj2'"} {>child obj=value/}
{:else}
{key}:{value}
{/if}
{~n}{/iter}

“child” partial to handle child level
{@iter key=obj}child: {key}:{value}{~n}{/iter}

Data model:
obj: {
a: "A",
b: "B",
obj2: {
a1: "A1",
b1: "B1"
},
c: "C"
}

If you are looking for a fully general solution to handle sub-objects with no prior knowledge of their key name (e.g, maybe writing some sort of hierarchical object dumper), here is a solution. It feels a bit hacky as it depends on object.toString yielding [object Object] but this is part of the standard so it should be valid and portable.

child partial:
{@iter key=obj}
{@if cond="'{value}' == '[object Object]'"} {>child obj=value/}
{:else}
{key}:{value}
{/if}
{~n}
{/iter}

Invocation on object from the data model:
{>child obj=obj/}

@rragan
Copy link
Contributor

rragan commented Jun 18, 2013

Had some more thoughts on this over the weekend and played around with a new version. It has an optional sort parameter. If you specify sort="" you get the default JS sort. If you specify sort="name", then context.global is checked for a function with that name which should implement what compare you desire. Adding sort provides a browser-independent version of iteration so a user is assured of the same answers. I left the no sort version around if it happens to meet your needs as it would be faster.

I also added "type" to the key and values pushed to the stack. This makes it cleaner to handle nested objects when you want to do something different for one of the substructures. Note the change from chunk.render to bodies.block. Since chunk.render just calls body(chunk, context) this ought to be marginally faster.

    "iter": function (chunk, context, bodies, params) {
        params = params || {};
        var obj = params['key'] || context.current();
        if (typeof params.sort !== "undefined") {
            var arr = [];
            for (var k in obj) {
                if (obj.hasOwnProperty(k)) {
                    arr.push(k);
                }
            }
            if (context.global[params.sort]) {
                arr.sort(context.global[params.sort]);
            } else {
                arr.sort();
            }
            for (var i = 0; i < arr.length; i++) {
                chunk = bodies.block(chunk, context.push({key: arr[i], value: obj[arr[i]], type: typeof obj[arr[i]]}));
            }
        } else {
            for (var k in obj) {
                if (obj.hasOwnProperty(k)) {
                    chunk = bodies.block(chunk, context.push({key: k, value: obj[k], type: typeof  obj[k]}));
                }
            }
        }
        return chunk;
    }

Thoughts?

@brikis98
Copy link

@rragan I like it. It seems fairly clean and quite useful, both for the simple case (I don't care about order) and the more complicated ones.

Only two questions I would have are:

  1. Should key and value be pushed onto the context as regular key/values or should they be handled by helpers similar to {@idx/}? I just worry that key and value are common key names and likely to shadow something you want access too. Another option may be to use __key__ or some other relatively unique name.
  2. Should the sort parameter be accompanied by an order parameter that accepts asc and desc values?

@rragan
Copy link
Contributor

rragan commented Jun 18, 2013

I agree that the names should be $key, $value, $type to stay out of the way of user names and in the pattern of $idx.

Compare functions are easy to define, viz.
asc: function asc (a, b) {return a > b ? 1 : a < b ? -1 : 0;},
desc: function desc (a, b) {return a < b ? 1 : a > b ? -1 : 0;}

As written, using sort="" gives you the default JavaScript sort order which is ascending. I guess it boils down to whether we supply a default choice for a descending sort. I guess sort="asc" and sort="desc" could be "built-in" options if it was thought useful enough.

@rashad612
Copy link
Author

Do you think assuming current context "context.current()" is a good practice, or should be omitted ?

@rragan
Copy link
Contributor

rragan commented Jun 18, 2013

I'm inclined to omit it particularly since setting context to an object via {#obj} is generally not useful -- part of the muddy waters around dual use of {#xxx} for iteration and context setting.

@rragan
Copy link
Contributor

rragan commented Jun 18, 2013

Here is an updated version supporting ascending and descending sorts built-in and changed to use $key, $value, $type names. Helper name is up for grabs. Some have used iter while I'm sure those favoring readability over a few key strokes will prefer "iterate".

  /**
     * iter helper, loops over given object.
     * Inspired: https://github.com/akdubya/dustjs/issues/9
     *
     * Example:
     *    {@iter key=obj}{$key}-{$value} of type {$type}{~n}{/iter}
     *
     * @param key - object of the iteration - Mandatory parameter
     * @param sort - Optional. If omitted, no sort is done. Values allowed:
     *               sort="" - sort ascending (per JavaScript array sort rules)
     *               sort="desc" - sort descending
     *               sort="fname" - Look for fname object in global context,
     *                  if found, treat it as a JavaScript array sort compare function.
     *                  if not found, result is undefined (actually sorts ascending
     *                  but you should not depend on it)
     */
    "iter": function (chunk, context, bodies, params) {
        params = params || {};
        var body = bodies.block;
        if (typeof params.key !== "undefined") {
            var obj = dust.helpers.tap(params.key, chunk, context);

            if (body) {
                if (typeof params.sort !== "undefined") {
                    var sort = dust.helpers.tap(params.sort, chunk, context);
                    var arr = [];
                    for (var k in obj) {
                        if (obj.hasOwnProperty(k)) {
                            arr.push(k);
                        }
                    }
                    var compareFn = context.global[sort];
                    if (!compareFn && sort === "desc") {
                        compareFn = desc;
                    }
                    compareFn ? arr.sort(compareFn) : arr.sort();
                    for (var i = 0; i < arr.length; i++) {
                            chunk = body(chunk, context.push({$key: arr[i], $value: obj[arr[i]], $type: typeof  obj[arr[i]]}));
                    }
                } else {
                    for (var k in obj) {
                        if (obj.hasOwnProperty(k)) {
                            chunk = body(chunk, context.push({$key: k, $value: obj[k], $type: typeof  obj[k]}));
                        }
                    }
                }
            } else {
                _console.log("Missing body block in the iter helper.");
            }
        } else {
            _console.log("Missing parameter 'key' in the iter helper.");
        }
        return chunk;

        function desc(a, b) {
            return a < b ? 1 : a > b ? -1 : 0;
        };
    }

@brikis98
Copy link

Looks pretty good to me. Naming is, of course, the most difficult part :)

Other alternatives to consider are @forEach and @loop.

@rashad612
Copy link
Author

how about iterate ? :)

@rragan
Copy link
Contributor

rragan commented Jun 18, 2013

iterate it is unless others feel differently. forEach seems a bit wrong as it really is a for..in loop. Loop seems a bit to general and inviting expectations beyond what it does.

@vybs
Copy link
Contributor

vybs commented Jun 18, 2013

iterate for me too.

@brikis98
Copy link

Works for me.

@rragan rragan mentioned this pull request Jun 20, 2013
@rragan
Copy link
Contributor

rragan commented Jun 20, 2013

Submitted alternate pull request #49 in the style discussed here along with tests. They are pretty similar at this point except for a bunch of tests in the other PR to test all the use cases and error cases. Good joint work from all to refine this.

@rragan
Copy link
Contributor

rragan commented Nov 18, 2013

This has lingered long enough. I am assuming there is little sentiment for putting this in the main dustjs-helpers.

I have captured the implementation in a separate repo at

https://github.com/rragan/dust-motes/tree/master/src/helpers/control/iterate

and it is available as an npm module: require('dustmotes-iterate')

Having worked hand-in-hand with the original contributor, I will leave the final decision to accept this PR or close it to someone else. There is now an alternative though.

@sethkinast sethkinast closed this Mar 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants