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

Guards, numbers and units #1180

Closed
SomMeri opened this issue Feb 12, 2013 · 7 comments
Closed

Guards, numbers and units #1180

SomMeri opened this issue Feb 12, 2013 · 7 comments

Comments

@SomMeri
Copy link
Member

SomMeri commented Feb 12, 2013

Hi, I found three issues on guards units handling.

1.) The equals = operator does not compare numbers with the same units. Expression 4px=4px is evaluated as false.

Example:

 .mixin(...) {
  catch:all;
 }
 .mixin(@var) when (@var=4) {
   declare: 4;
 }
 .mixin(@var) when (@var=4px) {
   declare: 4px;
 }
 #tryNumberPx {
   .mixin(4px);
 }

compiles into:

#tryNumberPx {
  catch: all;
  declare: 4;
}

expected output:

#tryNumberPx {
  catch: all;
  declare: 4;
  declare: 4px;
}

2.) The equals = operator both can and can not compare unitless numbers with units. Expression 4px=4 is evaluated as true, but 4=4px is false.

Example input:

 .mixin(@var) when (@var=4px) {
   declare: pixels;
 }
 .mixin(@var) when (4px=@var) {
   declare: pixels reversed;
 }
 #tryNumber {
   .mixin(4);
 }

compiles into:

#tryNumber {
  declare: pixels reversed;
}

expected output:

#tryNumber {
  declare: pixels;
  declare: pixels reversed;
}

3.) Operators '<', '<=', '>', '>=' ignore units entirely. The expression 3%>=2px evaluates to true. Equals '=' on numbers with different units evaluate to false.

Example:

 .mixin(@var) when (@var>=2px) {
   declare: pixels;
 }
 #tryNumber {
   .mixin(3%);
 }

compiles into:

 #tryNumber {
   declare: pixels;
 }

expected output:

#tryNumber {   }  // basically nothing

Tested on less-1.4.0-alpha.js and less-1.3.3.js.

@lukeapage
Copy link
Member

  1. is a 1.4.0 regression - high priority to sort out

(2) & (3) it would make sense I think to ignore units if one doesn't have units, otherwise always return false, so I agree

@SomMeri
Copy link
Member Author

SomMeri commented Feb 28, 2013

I found another asymmetry in equals operator handling:

4.) The equals = operator both can and can not compare escaped values with identifiers. Expression ~"identifier"=identifier is evaluated as true, but identifier=~"identifier" is false. Both should evaluate to the same value. I think it should be true in both cases, because ~"identifier"=identifier is tested in unit tests.

Example input:

.mixin(@str) when (~"theme1" = @str) { matched: escape is identifier; }
.mixin(@str) when (@str = ~"theme1") { matched: identifier is escape; }
.stringguardtest {
  .mixin(theme1);
}

compiles into:

.stringguardtest {
   matched: escape is identifier;
}

Expected output:

.stringguardtest {
   matched: escape is identifier;
   matched: identifier is escape; 
}

This does not match description "Guards, numbers and units", but seems to be related.

@lukeapage
Copy link
Member

I've fixed the regression (1)

(2) & (3) need more thought so am deferring for a later release as I think it is workable in its current state, if not consistent.

@seven-phases-max
Copy link
Member

Btw, (2) & (3) were fixed in 1.7.0 so I guess this can be closed?

@lukeapage
Copy link
Member

Makes sense. Any idea if 4 is fixed too?

@seven-phases-max
Copy link
Member

Ah, I've overlooked the (4). Nope, it is not fixed... I guess that's just because unlike other cases this one compares different types, i.e. keyword with quoted and the left-side keyword expects only other keyword values (while left-side quoted compares to any value via toCSS).

Btw, but the same applies to any type except quoted, e.g.:

.mixin(@v) when (~"1" = @v) {matched: escape is number}
.mixin(@v) when (@v = ~"1") {matched: number is escape}
.test {
  .mixin(1);
}

I.e. we probably can make one universal compare function for all of those types, but the escaped quoted still remains weird type, e.g.:

.mixin(@v) when (@v = 1) { // so quoted is a number?
    value: (1 + @v); // Err., oops, it is not actually :)
}

.test {
  .mixin(~"1");
}

@seven-phases-max
Copy link
Member

Closing as case (4) is fixed in v2.

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

Successfully merging a pull request may close this issue.

3 participants