Fixes #2270 - Adds each() function to Less functions #3263

matthew-dean · 2018-07-04T16:14:00Z

calvinjuarez

Failed Travis CI build for Node 8?

calvinjuarez · 2018-07-07T01:53:58Z

lib/less/functions/list.js

+    return items;
+};
+
+functionRegistry.addMultiple({


Love the movement of the functions into the file where they're most relevant!

calvinjuarez · 2018-07-07T02:04:39Z

lib/less/functions/list.js

+                key = new Dimension(i);
+                value = item;
+            }
+            newRules.push(new Declaration('@value',


Since these are special reserved variables now, I wonder if we make them less likely to conflict with variable names developers might be using? Like @__value or @VALUE? Is there precedent for reserved variable names?

On a related note, I know "key" and "index" have subtle flavor differences, but I wonder if we minimize reserved variables by just using either one or the other. Is there a case where we'd need both that I'm overlooking?

Well:

a) These vars are only bound specifically within the context of this function, so how would this possibly conflict?
b) Less already binds an @arguments in the context of mixins, so it would be strange to start suddenly namespacing injected vars in that way.

On a related note, I know "key" and "index" have subtle flavor differences, but I wonder if we minimize reserved variables by just using either one or the other. Is there a case where we'd need both that I'm overlooking?

You can iterate an each() over a ruleset, which will then have, for each entry (declaration), a @key (property or var name), @value (value), and @index (numerical position). Unless it should be @name instead of @key ?

Less already binds an @arguments in the context of mixins...

Precedent wins for me.

I think @name is probably more likely to have some global value assigned to it by authors, so I think @key is probably better. This would be the only kind of conflict I could see happening with any of these reserved variables.

Would there be any value in warning an author when s/he assigns a value to a variable that shares its name with a "reserved" var? Nothing that would block compiling, but just to make the dev aware?

TL;DR: I'm convinced. No changes needed here. My only other thought would be @i instead of @index, maybe.

Along similar lines, I think better to be explicit (@index vs @i, and also precedent of full names)

Besides; if an author desperately wants a short-hand because their loop-body contains long lines of code they want to condense: nothing is preventing them from doing a quick @i : @index, right?

And for formal parameters, it's better to have formal names.

And for formal parameters, it's better to have formal names.

Yep.

calvinjuarez · 2018-07-07T02:08:41Z

test/less/functions-each.less

+    @i: (@index - 1);
+
+    each(@value; {
+      @a: extract(@alpha, (@i * 2 + @key));


I'm having trouble following this test. Is it to test calculations with index and key? Maybe we split it into 2 tests, one to test loops in loops, and another to test calculations? Or we could just add comments to clarify what's being tested.

Yeah, that actually threw me when I went back and saw it, even though I wrote it (a couple years ago). Basically it's just demoing that you can get a reference the scoped index of parent in a nested loop. So in an loop of a loop, you can get parent values / indexes along with local by assigning them to a different var in the parent.

Hmmm... 🤔 this won't actually work the way I had it before, because of lazy evaluation. If you do @i: @index for example, it's evaluated when called.

So I'm wondering what to do about this. Technically you can nest each(), but if you can't specify your var names, and vars are lazy evaluated, then you can't reference the "parent" index/key/value. Does that matter?

Hmm...yeah, loops would need to expose them as different variables...maybe we expose each as a list/map?

each(@list1, { @value-loop1: @each[@1][value]; // or @each[@1][] @key-loop1: @each[@1][key]; @index-loop1: @each[@1][index]; // or @i-loop1: @each[@1][i]; each(@list2, { @value-loop2: @each[@2][value]; // or @each[@2][] @key-loop2: @each[@2][key]; @index-loop2: @each[@2][index]; // or @i-loop2: @each[@2][i]; }) })

Maybe you need to go the generic route and add a function that can be used to selectively force in-place/eager evaluation? Would maybe help with the &() feature as well, since that also needs to be eager.

Maybe you need to go the generic route and add a function that can be used to selectively force in-place/eager evaluation? Would maybe help with the &() feature as well, since that also needs to be eager.

Can you clarify? Note that I wrote this comment before defining / implementing the anonymous mixin syntax. I really don't think we want to change the behavior of scoped vars.

Just something that I was musing about.

There is - in a way - a method already to force eager evaluation of a variable; it has to be done before offering the variable's value to the function in a function call node. The 'problem' there is that the evaluation of the actual function to which the variable is passed, is still lazy.

i.e. in @a: foo(@b) @b might be eagerly evaluated when foo executes, but the execution of foo itself is still lazy and depends on tapping / accessing @a.

But if there were a way to force the evaluation of a function like foo to be internally eager and just cache its evaluated value until its pulled for by a lazy execution of a larger expression or chain of variable assignments in which foo(@b) partakes, then in effect @b would have been evaluated eagerly.

It means you would be able to force-eval loop variables at the correct time and possibly support other interesting / complex scenarios that have problems with lazy evaluation right now.

Ofcourse; with custom parameter names now introduced to each(), these problems should be resolved already. There is no longer a lazy chain of @index (or @key or @value) variable-aliasing assignments to walk through. So the point - atleast for this PR - no longer applies.

Okay, yeah for this one, it's no longer an issue once you can name vars.

Do you approve of this PR as written then?

@calvinjuarez Also it would be good to get your sign-off if good too, since it's a new syntax construct for Less. If there's hesitation about it, let me know.

matthew-dean · 2018-07-08T02:11:58Z

@calvinjuarez

See new tests and parser updates.

  // nested each
  each(10px 15px, 20px 25px; { 
    // demonstrates nesting of each()
    each(@value; #(@v, @k, @i) {
      nest-@{i}-@{index}: @v @k;
    });
  });

produces:

  nest-1-1: 10px 1;
  nest-2-1: 15px 2;
  nest-1-2: 20px 1;
  nest-2-2: 25px 2;

calvinjuarez

Do nested .() {} loops work? If so, we should add a test testing that .() defines unique mixins each time you nest.

test/less/functions-each.less

+      nest-@{i}-@{index}: @v @k;
+    });
+  });
+


test/css/functions-each.css

+  nest-1-2: 20px 1;
+  nest-2-2: 25px 2;
+  padding: 10px 20px 30px 40px;
+}


calvinjuarez · 2018-07-11T01:53:44Z

This is ready. My final question is just whether the reserved variables @value, @index, and @key are necessary at this point?

To be clear, though, this isn't a blocking comment/discussion by any means. This is good to go.

matthew-dean · 2018-07-11T02:32:35Z

@calvinjuarez I think the injected vars are nice to have? It just limits the amount of boilerplate you have to write.

matthew-dean added 4 commits July 3, 2018 23:05

WIP each() re-implementation

7a548a6

Added each() and tests

15f7b1a

Merge branch 'master' into feature/each-2

31b73b3

Added tests calling mixins from each()

a95823d

matthew-dean mentioned this pull request Jul 4, 2018

Allow parametrized mixins as detached rulesets to form 'lambdas' #2270

Closed

calvinjuarez requested changes Jul 7, 2018

View reviewed changes

calvinjuarez reviewed Jul 7, 2018

View reviewed changes

Merge branch 'master' into feature/each-2

defc03f

matthew-dean added the don't merge label Jul 7, 2018

Allows named args for detached ruleset (anonymous mixin)

3c7ccdb

matthew-dean removed the don't merge label Jul 8, 2018

rjgotten approved these changes Jul 10, 2018

View reviewed changes

Added .() example to each

e1e1c39

matthew-dean closed this Jul 10, 2018

matthew-dean reopened this Jul 10, 2018

calvinjuarez requested changes Jul 10, 2018

View reviewed changes

test/less/functions-each.less

nest-@{i}-@{index}: @v @k;

});

});

This comment was marked as outdated.

Sign in to view

test/css/functions-each.css

nest-1-2: 20px 1;

nest-2-2: 25px 2;

padding: 10px 20px 30px 40px;

}

This comment was marked as outdated.

Sign in to view

calvinjuarez added 2 commits July 10, 2018 19:08

add explicit nested anonymous mixin test

7104c3c

add explicit nested anonymous mixin test result

c49b11e

calvinjuarez approved these changes Jul 11, 2018

View reviewed changes

derp: align test with expected output

c974d4e

matthew-dean merged commit 3f82ef5 into less:master Jul 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #2270 - Adds each() function to Less functions #3263

Fixes #2270 - Adds each() function to Less functions #3263

matthew-dean commented Jul 4, 2018 •

edited

Loading

calvinjuarez left a comment

calvinjuarez Jul 7, 2018

matthew-dean Jul 7, 2018

calvinjuarez Jul 7, 2018

matthew-dean Jul 7, 2018 •

edited

Loading

calvinjuarez Jul 8, 2018 •

edited

Loading

matthew-dean Jul 8, 2018

rjgotten Jul 9, 2018 •

edited

Loading

matthew-dean Jul 9, 2018

calvinjuarez Jul 7, 2018

matthew-dean Jul 7, 2018

matthew-dean Jul 7, 2018

matthew-dean Jul 8, 2018

calvinjuarez Jul 8, 2018

rjgotten Jul 9, 2018 •

edited

Loading

matthew-dean Jul 9, 2018

rjgotten Jul 9, 2018 •

edited

Loading

matthew-dean Jul 9, 2018

matthew-dean Jul 9, 2018

matthew-dean commented Jul 8, 2018

calvinjuarez left a comment

This comment was marked as outdated.

This comment was marked as outdated.

calvinjuarez commented Jul 11, 2018 •

edited

Loading

matthew-dean commented Jul 11, 2018

Fixes #2270 - Adds each() function to Less functions #3263

Fixes #2270 - Adds each() function to Less functions #3263

Conversation

matthew-dean commented Jul 4, 2018 • edited Loading

calvinjuarez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-dean Jul 7, 2018 • edited Loading

Choose a reason for hiding this comment

calvinjuarez Jul 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjgotten Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjgotten Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjgotten Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-dean commented Jul 8, 2018

calvinjuarez left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

calvinjuarez commented Jul 11, 2018 • edited Loading

matthew-dean commented Jul 11, 2018

matthew-dean commented Jul 4, 2018 •

edited

Loading

matthew-dean Jul 7, 2018 •

edited

Loading

calvinjuarez Jul 8, 2018 •

edited

Loading

rjgotten Jul 9, 2018 •

edited

Loading

rjgotten Jul 9, 2018 •

edited

Loading

rjgotten Jul 9, 2018 •

edited

Loading

calvinjuarez commented Jul 11, 2018 •

edited

Loading