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

Initial support for custom parsed functions (boolean, if etc.) #3079

Merged
merged 1 commit into from
Jun 7, 2017

Conversation

seven-phases-max
Copy link
Member

@seven-phases-max seven-phases-max commented Jun 7, 2017

So basically this is a facility for functions with arguments not following the ordinal value parsing rules.
Two such functions added with this PR, if and boolean (per #3019), i.e.:

div {
    color: if(not(2 < 1), blue, green);
    @var: boolean((4 = 2) or (true));
    font-size: if((@var)), 42px, 24px);
    // etc.
}

Known Issues / Questions / Needs-Decisions:

[1]

Surplus parens. Notice the boolean expressions (except logical ops, i.e.: not/and/or) require extra parens:

if((2 > 1), blue, green); // OK
if(2 > 1, blue, green);   // Error

This is because the code uses the parsing leaf of the when expressions and there the parens grew inwards quite deeply. Obviously this is ideally to be improved before release, so considering possible workarounds for now - related to #2807.

[2]

Ruleset args and non-rule-value calls. Consider:

div {
    if(not(true),
        {padding: 0},
        {margin:  0}
    );
}

Well, this is not possible with this PR because returned DR values are not allowed (DR itself has no related code anyway) to do anything if appear naked at the block level. So to achieve above goal one has to do:

div {
    @result: if(not(true),
        {padding: 0},
        {margin:  0}
    );
    @result();
}

instead.
I'm going to enable the non-@result-bloating variant too by automatically expanding returned DR values for the ruleset level function calls, but for a moment it's a bit difficult due to ruleset evaluation code trickery. On one hand this would smell pretty much like a guesswork-behaviour (I usually oppose to), on the other hand - in this particular case, imho, - the auto-expansion would perfectly fit the original spirit of the #965 thing.
After all, the very idea of @var() syntax came only to prevent @var; ambiguity because inside mixins the ruleset is already assigned to a variable anyway. But for the above div { if(not(true), ... code where no @var is initially involved no such ambiguity can exist (unless I'm missing something) - so it will do basically just what's written: "ruleset-in -> set-of-rules-out".


[3]

P.S. Ah, and yes, no space allowed between if and () - this is quite annoying but it's basically a Less/CSS function requirement, we do not want to introduce a new parsing-concept/API-node just for that space, do we? (Hmm, on the other hand though, some quick and dirty hack to allow this won't be that difficult if necessary).

@seven-phases-max seven-phases-max merged commit 257f615 into 3.x Jun 7, 2017
@seven-phases-max seven-phases-max deleted the special-funcs branch June 7, 2017 17:15
@matthew-dean
Copy link
Member

Nice work!

Ah, and yes, no space allowed between if and () - this is quite annoying but it's basically a Less/CSS function requirement

There's no reason to change this requirement.

The requirement for extra parens is unfortunate, but from what I know of the codebase, I get why this would be a challenge on the first pass. I agree it would be ideal to remove this requirement in the future, but I wouldn't see it as holding up this release.

I'm going to enable the non-@result-bloating variant too by automatically expanding returned DR values for the ruleset level function calls. On one hand this would smell pretty much like a guesswork-behaviour (I usually oppose to), on the other hand - in this particular case, imho, - the auto-expansion would perfectly fit the original spirit of the #965 thing.
After all, the very idea of @var() syntax came only to prevent @var; ambiguity because inside mixins the ruleset is already assigned to a variable anyway. But for the above div { if(not(true), ... code where no @var is initially involved no such ambiguity can exist (unless I'm missing something) - so it will do basically just what's written: "ruleset-in -> set-of-rules-out".

This is a good idea, and actually could help plugin functions quite a bit. When I added a bunch of tests for plugins, I realized there was no easy way to just return a set of rules. (For example, for a case in which a plugin function doesn't want to return just one node, but multiple nodes.) The only possible solution for me was to basically do what you had to do: 1) return a var pointing to a DR, 2) add a DR call for that var.

Would it be reasonable then to allow in the future:

.box {
  // vs   & { } for scope isolation
  {  
    @import "colors";
    color: @color;
  }
  @color: red;
}

That is, allow a detached ruleset without a & selector? Because isn't that what your if() statement is returning to the parent block? If it's a valid return value for a function, then it logically follows that it can be declared outside of a function, otherwise the functions would have to return & { ... }. Correct?

@seven-phases-max
Copy link
Member Author

seven-phases-max commented Jun 7, 2017

Because isn't that what your if() statement is returning to the parent block?

Currently I'm considering just to generate an equivalent of @dr() w/o introducing any new stuff.
But, yes indeed, if it works like that then in your example that {} would naturally do the same if supported by parser (though I wonder if it's worth to add new parsing code if we already have & {}?)

@seven-phases-max
Copy link
Member Author

I removed the dedicated tree type completely. Now its handled via regular "escaped string" type (tree.Quoted).

@matthew-dean
Copy link
Member

I removed the dedicated tree type completely. Now its handled via regular "escaped string" type (tree.Quoted).

Makes sense. 👍

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.

2 participants