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

Fix mathematical operations with different units #1049

Merged
merged 3 commits into from
Apr 30, 2015

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Apr 6, 2015

Fix mathematical operations with different units:

foo { bar: (1cm*2in/1cm); }

Could move convert function back to normalize, but I don't think we pay much in terms of performance for keeping them apart for now. Made quite a few wrong turns while implementing that, since I intially wanted to use a map table for the units (like counting exponents). But then it was not able to normalize units the way ruby sass does, as insert order is important for that.

So this is not just a cosmetic fix, libsass also calculated wrong results in certain situations (see changes in eval.cpp). I do hope this is on par with ruby sass now 😉

Makes it pass this spec test:

  • libsass/rel
  • libsass/list
  • libsass/conversions

IMO it's only missing (optional) performance optimizations!

@mgreter mgreter added this to the 3.2.1 milestone Apr 6, 2015
@mgreter mgreter self-assigned this Apr 6, 2015
@mgreter mgreter force-pushed the bugfix/math-ops-with-units branch 4 times, most recently from 632dfca to eed6698 Compare April 6, 2015 21:41
@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 80.56% when pulling eed6698 on mgreter:bugfix/math-ops-with-units into f82a41b on sass:master.

This was referenced Apr 30, 2015
@xzyfer xzyfer modified the milestones: 3.2.1, 3.2.2 Apr 30, 2015
@mgreter mgreter force-pushed the bugfix/math-ops-with-units branch from eed6698 to b5ac186 Compare April 30, 2015 01:25
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 80.28% when pulling b5ac186 on mgreter:bugfix/math-ops-with-units into 9ad775f on sass:master.

xzyfer added a commit that referenced this pull request Apr 30, 2015
Fix mathematical operations with different units
@xzyfer xzyfer merged commit 389bc92 into sass:master Apr 30, 2015
@mgreter mgreter deleted the bugfix/math-ops-with-units branch July 28, 2015 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants