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

Odd Argument behavior #35

Closed
dylan opened this issue Jun 2, 2010 · 41 comments
Closed

Odd Argument behavior #35

dylan opened this issue Jun 2, 2010 · 41 comments

Comments

@dylan
Copy link

dylan commented Jun 2, 2010

I have this:
.box-shadow (@x){
box-shadow: @x;
-webkit-box-shadow: @x;
-moz-box-shadow: @x;
}

When I do this:
.box-shadow(0 0 2px @second-color,inset 0 0 2px @second-color);

Everything renders correctly, but when I do this:
.box-shadow(0 2px 5px rgba(0, 0, 0, 0.125), 0 2px 10px rgba(0, 0, 0, 0.25));

It drops the second attribute. I have tried it without a space after the comma too.

@cloudhead
Copy link
Member

hmm, the correct behaviour is the 2nd one though, , is the argument delimiter.

@dylan
Copy link
Author

dylan commented Jun 2, 2010

So how would you recommend passing several (n number) arguments to box-shadow for css3?

@jamesfoster
Copy link
Contributor

I'd recommend using variables:

@var: 0 0 2px @second-color,inset 0 0 2px @second-color;
.box-shadow(@var);

i don't think we can support passing values with commas. I also don't like the idea of escaping the comma with \, or ,,

another (ugly) way to do it would be to pass the value as a "string" and unquote it inside the mixin using e(@x).

I'd say the first case is a bug. it should pass 2 arguments

@cloudhead
Copy link
Member

what he said.

@Wyverald
Copy link

Wyverald commented Dec 6, 2010

Sometimes I'd think that using the semicolon (;) as the argument delimiter would be syntactically more consistent. Think about it - using commas for delimiting both mixin arguments and parts of some particular style rules (background, box-shadow, etc., but most notably font-family) can be inconsistent/ambiguous at times, resulting in cases like this.

Semicolons, however, have always been used to signify the end of a style rule (and also the end of a variable value in LESS), so it fits pretty well in the role of delimiting arguments. As a bonus, this even makes it possible to give default argument values that contain commas (for a trivial example .otherfont (@face: Helvetica, Arial; @size: 13px) { font-family: @face; font-size: @size; }). This is really not that unintuitive.

If you wrap your head around it, this could prove the best solution - if not for the fact that it might break existing code. To solve this problem, some sort of compilation configuration flags could be set to determine whether ',' or ';' would be used as argument delimiters, with the former being the default.

@cloudhead
Copy link
Member

I think you make a good point for semicolons—The problem is more to do with what people are used to, when passing arguments to a function, in any other language. I'll see what I can do, in terms of supporting both styles. In the end, the syntax which allows the most flexibility, ie semicolons should win.

@fabiosantoscode
Copy link

less is just treating the values after the comma as a second argument.

@matthew-dean
Copy link
Member

About this:

Sending a comma separated value to a single variable can be done like this:

.box-shadow(~"0 2px 5px rgba(0, 0, 0, 0.125), 0 2px 10px rgba(0, 0, 0, 0.25)");

I do think supporting both semi-colons and commas is actually a decent compromise, because semi-colons are not accidental insertions (not inserted into a mixin statement for any other reason that I can think of).

The logic could be as follows:

  • Both commas and semi-colons are argument delimeters, unless...
  • If both are present, treat only semi-colons as an argument delimeter

This would allow all existing LESS code to function exactly the same, and extend the ability to separate arguments that may have commas without resorting to string escaping. I think this is a good suggestion, and deals more elegantly with the problem that CSS presents, that the "value" of a property (and thus a variable) can have white space and commas already.

UPDATE: One more argument in support of this: the English language. Lists have commas unless the object listed contains a list, in which case the list is separated by semi-colons. So @Wyverald has a good example, but I'm against forcing a compilation flag. We can easily do it automatically and intuitively by allowing a dev to choose semi-colons instead of commas for any given mixin.

@fabiosantoscode
Copy link

+1. Reminds me of javascript strings. Use "" or '', whatever fits your needs.

@matthew-dean
Copy link
Member

One revision. My previous solution won't work (EDIT: Maybe, as there are possibly other ways to indicate a "switch" to semi-colons). If there's only one argument, that algorithm wouldn't be able to detect if the comma was intended as part of the argument or a second argument.

SASS handles this more elegantly and intuitively. The syntax uses parentheses.

The syntax would be like this:

.box-shadow( ( 0 2px 5px #FFF , 0 2px 10px #000 ) );

Each argument can be wrapped in parentheses to indicate the entire sequence is one argument, and each set of parentheses can then separated by commas, like this:

// definition
.css3mixin(@boxShadow, @borderRadius) {}

//usage
.css3mixin( ( 0 2px 5px #FFF ,  0 2px 10px #000 ), 3px );

LESS is already using parentheses to optionally wrap expressions, so it doesn't introduce any new syntax and follows convention.

@matthew-dean
Copy link
Member

Coming back to this, this does get a little complicated to use now that parentheses is being used to indicate math operations. Don't know if that will make this too complicated or we can still make the parser smart enough.

For example:

.border-radius(@radius) {
  -webkit-border-radius: @radius;
}
.myclass {
  .border-radius( (10px / 2px) );
}

Now, was the author's intent to specify a valid elliptical radius as simply the first (and only) argument? Or was their intent to do math division? How is the parser supposed to know?

I want LESS to be completely backwards-compatible, but in truth, if I had to make this choice before LESS was in widespread use, I probably would have advocated instead for semi-colon separators, as it's much cleaner and clearer, doesn't conflict with CSS, is easier to understand, and doesn't give a possible unexpected result like the above example. But I hate breaking things, so I'm torn.

Food for thought. Would love to hear more input.

@lukeapage
Copy link
Member

I would phrase the concern as one for consistancy.. e.g.

.border-radius( (10px / 2px) );  -> .border-radius(5px);

.border-radius( (10px / 2px, 1) ); -> .border-radius(~"10px / 2px, 1");

is that obvious? I think the parser could handle these two differently easy enough, but would it be obvious what is going to happen?

@lukeapage
Copy link
Member

In summary

@arguments-list - hack and incomplete
comma-seperated(@arguments) - hacky
parenthesis means keep "," literal - not so obvious in some situations
change to ";" for seperating args - cleanest but breaking change

? @cloudhead any thoughts?

@matthew-dean
Copy link
Member

Right, I get you. In case of a conflict between parentheses for specifying arguments and parentheses for math, we do math (but only if what's inside the parentheses is a valid math equation).

If you want to override this behavior (for this extreme edge case), you use string escaping.

My example is a bit of a red herring anyway, since it doesn't make sense that an author would arbitrarily be using a single set of parentheses for arguments.

Further to issue #146, here is a not-so-edge case with what I believe should be the intended behavior:

/* Example - Test A */

.mymixin(@font, @borderRadius) {
  font: @font;
  border: 1px solid red;
  border-radius: @borderRadius;
}
.myclass {
  .mymixin( ( 14px/1.4 Georgia, serif), 10px / 2px ); 
}

/* Expected output - Test A */

.myclass {
  font: 14px/1.4 Georgia, serif;  /* parentheses were not around a valid equation, so don't do math */
  border: 1px solid red;
  border-radius: 10px / 2px; /* no inner parentheses, so don't do math */
}

/* Forced division - Test B */

.myclass {
  .mymixin( ( (14px/1.4) Georgia, serif), (10px / 2px) ); 
}

/* Expected output - Test B */

.myclass {
  font: 10px Georgia, serif;  /* the valid equation was directly encapsulated by parentheses, meaning an intended math operation */
  border: 1px solid red;
  border-radius: 5px;  /* it's possibly an argument, but math wins */
}

/* Overriding when math/argument is ambiguous - Test C */

.myclass {
  .mymixin( ( (14px/1.4) Georgia, serif), (~"10px / 2px") ); 
}

/* Expected output - Test C */

.myclass {
  font: 10px Georgia, serif;  /* same as Test B */
  border: 1px solid red;
  border-radius: 10px / 2px;  /* rejected as a math operation, so the parentheses are an argument delimiter */
}

@lukeapage
Copy link
Member

I agree that this is how I would expect it to work if this is what we decide to do.

Sorry - another point.

parenthesis means breaking change for the consumer of the mixin. arguments-list/comma-seperated mean a optional change for the author of the mixin.

@matthew-dean
Copy link
Member

Nope, parentheses for arguments are not a breaking change. Parentheses are still optional. If we added parentheses-wrapped arguments today, nothing would break anywhere. I was only demonstrating the new division breaking change in Pull Request #915 to clarify the difference between arguments and math, but you should still be able to do this:

.mymixin(@border, @color) {
  border: @border;
  color: @color;
}
.classA {
  .mymixin(1px solid black, red);
}
.classB {
  .mymixin( (1px solid black), (red) );
}

/* Output */

.classA {
  border: 1px solid black;
  color: red;
}
.classB {
  border: 1px solid black;
  color: red;
}

So every LESS library everywhere would still work with clockwork with the proper code changes, which is why this one might be favored in the end (as opposed to semi-colons).

@leafo
Copy link

leafo commented Sep 5, 2012

I think the parentheses and math issue isn't really a problem. Think of (x+1) as a list with one element. When evaluating the expression and the list only has one element then we can just return that element instead of a list type. If we see something like (x+1, x+2) we can evaluate all the elements, see it has more than one and return a list type. Same applies for space separated list (x+1 x+2). Essentially everything is a list, SASS figured this out a lot time ago:

Lists are how Sass represents the values of CSS declarations like margin: 10px 15px 0 0 or font-face: Helvetica, Arial, sans-serif. Lists are just a series of other values, separated by either spaces or commas. In fact, individual values count as lists, too: they’re just lists with one item.

Also @MatthewDL the way less currently handles suppressing division has nothing to do with parentheses, but the name of the property the division takes place in:

div {
    border-radius: 10px/2px;
    font-size: 10px/2px;
    font: 10px/2px;
}

Compiles to:

div {
  border-radius: 5px;
  font-size: 5px;
  font: 10px/2px;
}

In order to pass around the slash syntax you would have to unquote a string.

(Another hack that crept its way in? See SASS's solution.)

@dmcass
Copy link
Contributor

dmcass commented Sep 5, 2012

I'm really not sure that's obvious enough. Also, you'd then have to nest parens 3 levels deep to accomplish division there and that feels really crowded, sloppy, and unnecessary. I don't want to introduce yet another breaking change, but I also really dislike the excessive parentheses for this syntax.

@matthew-dean
Copy link
Member

@dmcss That was my fear as well, that we're over-parenthesizing. What are you in favor of?

@dmcass
Copy link
Contributor

dmcass commented Sep 5, 2012

@leafo there's a commit coming that's going to require parens around division in all rules to avoid breaking CSS3 syntax for border-radius and background shorthand properties, and any future property that uses a slash as a delimiter. In addition, we've agreed (@cloudhead included) to eventually make all operations require parens to avoid future conflicts with CSS syntax like the calc function. This is the other breaking change I was talking about.

@MatthewDL I think the semi-colon solution feels the cleanest, but I haven't thought too deeply about what other options there are. Think about a mixin with named parameters.

.mixin(@foo: 10px; @bar: 3px) {    /* not a far cry from defining CSS rules */
  ...
}

The other option is square brackets, but I think that doesn't feel enough like CSS and still looks exceptionally crowded. I'll try to think of a few other ways to solve this problem.

@leafo
Copy link

leafo commented Sep 5, 2012

Well, if you don't mind breaking everyone's LESS then do semicolons. It's the correct solution in terms of syntax, but probably not the best idea given the adoption of ,.

@matthew-dean
Copy link
Member

@leafo I agree. Semi-colons are intuitively more "correct", but breaks more stuff. However, that's not necessarily a final argument against doing it, and @cloudhead has not been afraid to introduce breaking changes (and has weighed in on semi-colons being a possibility). In the long term, which solution are we going to be happier with?

If we did do semi-colons, then I think that might involve creating a LESS 2.0 branch, as that's a pretty significant, non-backwards-compatible change, so I welcome this active discussion. I'd want all sort of repercussions explored. Btw, thanks @leafo by linking in some of the SASS docs. It's interesting that we've often addressed the same problem with the same or a similar solution. It looks like they've done some great working thinking through the math-related questions, so maybe it's a place to look to just in terms of not having to completely reinvent the wheel from scratch. But of course, we'd still like to find our own way at the same time. ;-)

@lukeapage
Copy link
Member

by breaking change, I mean.. bootstrap would have to depreciate and then remove (or remove entirely)

.mixin(@arg1, @arg2) {
}

and therefore break people using

.mixin(a, b);

until they changed to

.mixin((a, b));

but its a very small consideration.

Over parenthesising is a concern... but I think we have to consider how many people will use this feature.. it isn't like every mixin call is going to be getting an extra parenthesis.. its going to be the odd one.. and here we are saying with the parenthesis that we are calling the mixin with a single argument, which does make it pretty clear to someone reading the less that it is only passing one argument.

Bit of an idea gamble here, but the ";" does not need to be a breaking change if we allow ";" at end any of a single argument, e.g. ";" are allowed as argument seperators and if you use ";" you automatically say that commas are passed whole, e.g.

.fire(one, two;) would -> .fire(~"one, two");

this could be implemented in the same way as parenthesis - a new node that handles ; seperated and if there is only one and it doesn't end in ";" then take the contents of the single node and expand as arguments.. with that fallback eventually being removed at some point in the future.

@lukeapage
Copy link
Member

I'm going round and round in my head over this.. I keep thinking "are the use cases big enough that we really need to consider this so much.. does arguments-list not just provide everything we would ever want"

but @MatthewDL since you speak with @cloudhead the most I'm really just happy to go with any consensus you to get to.

@matthew-dean
Copy link
Member

To your first example, no that's not correct. See my examples.

Having said that, your "idea gamble" is exactly what popped into my head when I was discarding the idea of using semi-colons in a way that is backwards compatible. Nice mind-meld. :-)

Earlier (above in this thread), I discarded a best of both worlds: use both commas and semi-colons. When semi-colons are present, then treat those as the argument separater. But in cases of single arguments, I felt we couldn't tell the parser if it was two arguments or one argument with a comma. But, to quote the epic Oscar-winning movie The Core, today I looked at this and said, "BUT WHAT IF WE COULD?"

Same as you just said. By terminating a single argument with a semi-colon, we effectively tell the parser that semi-colons are the separator.

To summarize:

  1. Everyone can keep using commas if they want.
  2. People can use semi-colons if they want.
  3. We don't over-use parentheses (and conflict with math)

Everyone wins. I'd call that a slam dunk @agatronic. HIGH FIVE.

@lukeapage
Copy link
Member

If we go with this I still think we should make efforts to depreciate "," and that is a hard sell.

@matthew-dean
Copy link
Member

So, to re-write my examples with semi-colon syntax:

/* Example - Test A */

.mymixin(@font; @borderRadius: 13px / 2px ) {  /* Note that semi-colons allowed here too */
  font: @font;
  border: 1px solid red;
  border-radius: @borderRadius;
}
.myclass {
  .mymixin( 14px/1.4 Georgia, serif; 10px / 2px ); 
}

/* Expected output - Test A */

.myclass {
  font: 14px/1.4 Georgia, serif;  
  border: 1px solid red;
  border-radius: 10px / 2px; 
}

/* Forced division - Test B */

.myclass {
  .mymixin( (14px/1.4) Georgia, serif; (10px / 2px) ); 
}

/* Expected output - Test B */

.myclass {
  font: 10px Georgia, serif;  
  border: 1px solid red;
  border-radius: 5px;  
}

/* Single argument - Test C */

.myclass {
  .mymixin( (14px/1.4) Georgia, serif; ); 
}

/* Expected output - Test C */

.myclass {
  font: 10px Georgia, serif;  
  border: 1px solid red;
  border-radius: 13px / 2px;  
}

Yeah, I think I like this better. Looks cleaner.

@matthew-dean
Copy link
Member

@agatronic Yes, we would change the documentation to favor semi-colons. But the GOOD part is that commas can still be accepted in legacy libraries without a problem.

@richardp-au
Copy link

What about using square or curly braces instead of parentheses to indicate a list?

Alternatively, if you went with parentheses, you could prefix it with a character to indicate the list. For example, *(a, b). This avoids any confusion between math and lists and it also bears a resemblance to string escaping with ~"...".

Just trying to throw out some other ideas. :)

@lukeapage
Copy link
Member

@R1ch0 curly brackets - we have enough of them, square brackets already discussed and I think discounted because they aren't in the flavour of declarative css..

add *( to parenthesis I think looks even more confusing (this vs "5 * (3 / 2)")

I think arguments-list and ; are my favourites

@matthew-dean
Copy link
Member

Just a note. Even though I called @arguments-list a hack, what I meant is that it didn't reach the underlying problem, and still left some problems unsolved and on the table.

Having said that, even though @arguments-list shouldn't be necessary with some fixes to argument separation, it doesn't mean it couldn't exist. In CSS, there's multiple ways to solve the same problem, and I'm fine with both solutions: a proper way to separate arguments, and a keyword for a comma-separated list. Are they mutually exclusive ideas?

-1 for curly or square brackets, for the same reasons @agatronic mentioned. I'm now leaning more heavily towards a backwards-compatible semi-colon syntax.

@richardp-au
Copy link

@agatronic Yeah I see that I picked a really bad example with *. But my point was simply that it could be prefixed with any character to indicate a list instead of math. Preferably a character that isn't used for other things! Oops.

I don't really have an opinion either way on this matter but if you use semicolons, wouldn't that mean it's not possible to assign a list to a variable? I know there's not really a need to do this since commas would work there, but it would create a small caveat that should be documented.

@matthew-dean
Copy link
Member

No, if you used semi-colons, it should still be perfectly possible to assign a list to a variable. Maybe I need to document it further.

@richardp-au
Copy link

@MatthewDL If I rewrite your example A above with a variable, would it still work?

.myclass {
  @var: 14px/1.4 Georgia, serif; 10px / 2px;
  .mymixin(@var); 
}

@lukeapage
Copy link
Member

It would not be possible to do this.. its not possible today right? Do we
need it? I hope not...

@richardp-au
Copy link

I can't see why you'd need or want that. I'm just saying that it should be clearly documented where semicolons are and aren't supported.

@dmcass
Copy link
Contributor

dmcass commented Sep 6, 2012

Semi-colons would only be allowed as delimiters of parameters in a parametric mixin definition or call.

And I'm on board with this, I still think it's the cleanest solution. The only other things I came up with were pipes as dilimeters (and or wrappers), and I still don't think I like that very much.

@matthew-dean
Copy link
Member

@R1ch0 Your example does not make sense. Even if you could declare a variable like that, which you can't, you would never be able to use it in LESS, since you can't just plop a variable onto it's own line. So....

@dmcass Thanks for the additional input. Yeah, I think we can really make this low impact. I like where this is headed. Who would like to start working on a PR request for this? ;-)

@lukeapage
Copy link
Member

I may have a go later afternoon - I have two flights and an idea on how to do it..

@lukeapage
Copy link
Member

Have almost finished - found 2 bugs with named arguments when writing unit tests so had to fix those too. Will put a pull request soon. Not completely sure on the syntax because the mix of , and ; doesn't exactly look completely explanatory.. but will send a pull and you can take a look.

@lukeapage
Copy link
Member

semi-colon seperators is checked into the master branch.

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

No branches or pull requests

10 participants