Cancel convertable units in the Number constructor. #180

6 files changed, 86 insertions, 24 deletions

diff --git a/scss/cssdefs.py b/scss/cssdefs.py
index 4a4b122..dd30f7f 100644
--- a/scss/cssdefs.py
+++ b/scss/cssdefs.py
@@ -184,6 +184,19 @@ BASE_UNIT_CONVERSIONS = {
     'dppx': (96, 'dpi'),
 }
 
+
+def get_conversion_factor(unit):
+    """Look up the "base" unit for this unit and the factor for converting to
+    it.
+
+    Returns a 2-tuple of `factor, base_unit`.
+    """
+    if unit in BASE_UNIT_CONVERSIONS:
+        return BASE_UNIT_CONVERSIONS[unit]
+    else:
+        return 1, unit
+
+
 def convert_units_to_base_units(units):
     """Convert a set of units into a set of "base" units.
 
@@ -203,6 +216,49 @@ def convert_units_to_base_units(units):
     return total_factor, tuple(new_units)
 
 
+def count_base_units(units):
+    """Returns a dict mapping names of base units to how many times they
+    appear in the given iterable of units.  Effectively this counts how
+    many length units you have, how many time units, and so forth.
+    """
+    ret = {}
+    for unit in units:
+        factor, base_unit = get_conversion_factor(unit)
+
+        ret.setdefault(base_unit, 0)
+        ret[base_unit] += 1
+
+    return ret
+
+
+def cancel_base_units(units, to_remove):
+    """Given a list of units, remove a specified number of each base unit.
+
+    Arguments:
+        units: an iterable of units
+        to_remove: a mapping of base_unit => count, such as that returned from
+            count_base_units
+
+    Returns a 2-tuple of (factor, remaining_units).
+    """
+
+    # Copy the dict since we're about to mutate it
+    to_remove = to_remove.copy()
+    remaining_units = []
+    total_factor = 1
+
+    for unit in units:
+        factor, base_unit = get_conversion_factor(unit)
+        if not to_remove.get(base_unit, 0):
+            remaining_units.append(unit)
+            continue
+
+        total_factor *= factor
+        to_remove[base_unit] -= 1
+
+    return total_factor, remaining_units
+
+
 # A fixed set of units can be omitted when the value is 0
 # See: http://www.w3.org/TR/2013/CR-css3-values-20130730/#lengths
 ZEROABLE_UNITS = frozenset((
diff --git a/scss/functions/core.py b/scss/functions/core.py
index b4a3a19..8e1c1ae 100644
--- a/scss/functions/core.py
+++ b/scss/functions/core.py
@@ -545,8 +545,8 @@ def _type_of(obj):  # -> bool, number, string, color, list
 
 @register('unit', 1)
 def unit(number):  # -> px, em, cm, etc.
-    numer = '*'.join(number.unit_numer)
-    denom = '*'.join(number.unit_denom)
+    numer = '*'.join(sorted(number.unit_numer))
+    denom = '*'.join(sorted(number.unit_denom))
 
     if denom:
         ret = numer + '/' + denom
diff --git a/scss/tests/files/bugs/009-division-in-assignment.scss b/scss/tests/files/bugs/009-division-in-assignment.scss
index df657f1..fd79971 100644
--- a/scss/tests/files/bugs/009-division-in-assignment.scss
+++ b/scss/tests/files/bugs/009-division-in-assignment.scss
@@ -1,4 +1,4 @@
-$em-scale: 12em / 1px;
+$em-scale: 12px / 1em;
 
 p {
     height: 0.5em * $em-scale;
diff --git a/scss/tests/functions/test_core.py b/scss/tests/functions/test_core.py
index c36250c..ce74da6 100644
--- a/scss/tests/functions/test_core.py
+++ b/scss/tests/functions/test_core.py
@@ -315,7 +315,9 @@ def test_unit(calc):
     assert calc('unit(100px)') == calc('"px"')
     assert calc('unit(3em)') == calc('"em"')
     assert calc('unit(10px * 5em)') == calc('"em*px"')
-    assert calc('unit(10px * 5em / 30cm / 1rem)') == calc('"em*px/cm*rem"')
+    # NOTE: the docs say "em*px/cm*rem", but even Ruby sass doesn't actually
+    # return that
+    assert calc('unit(10px * 5em / 30cm / 1rem)') == calc('"em/rem"')
 
 def test_unitless(calc):
     # Examples from the Ruby docs
diff --git a/scss/tests/test_expression.py b/scss/tests/test_expression.py
index 75a4dee..c496f79 100644
--- a/scss/tests/test_expression.py
+++ b/scss/tests/test_expression.py
@@ -92,7 +92,7 @@ def test_subtraction(calc):
 
 def test_division(calc):
     assert calc('(5px / 5px)') == Number(1)
-    assert calc('(5px / 5px)') == Number(1)
+    assert calc('(1in / 6pt)') == Number(12)
 
 def test_comparison_numeric(calc):
     assert calc('123 < 456')
diff --git a/scss/types.py b/scss/types.py
index 4f0984b..d2537a5 100644
--- a/scss/types.py
+++ b/scss/types.py
@@ -5,7 +5,7 @@ import operator
 
 import six
 
-from scss.cssdefs import COLOR_LOOKUP, COLOR_NAMES, ZEROABLE_UNITS, convert_units_to_base_units
+from scss.cssdefs import COLOR_LOOKUP, COLOR_NAMES, ZEROABLE_UNITS, convert_units_to_base_units, cancel_base_units, count_base_units
 from scss.util import escape
 
 
@@ -156,13 +156,27 @@ class Number(Value):
         if not isinstance(amount, (int, float)):
             raise TypeError("Expected number, got %r" % (amount,))
 
-        self.value = amount
-
         if unit is not None:
             unit_numer = unit_numer + (unit,)
 
-        self.unit_numer = tuple(sorted(unit_numer))
-        self.unit_denom = tuple(sorted(unit_denom))
+        # Cancel out any convertable units on the top and bottom
+        numerator_base_units = count_base_units(unit_numer)
+        denominator_base_units = count_base_units(unit_denom)
+
+        # Count which base units appear both on top and bottom
+        cancelable_base_units = {}
+        for unit, count in numerator_base_units.items():
+            cancelable_base_units[unit] = min(
+                count, denominator_base_units.get(unit, 0))
+
+        # Actually remove the units
+        numer_factor, unit_numer = cancel_base_units(unit_numer, cancelable_base_units)
+        denom_factor, unit_denom = cancel_base_units(unit_denom, cancelable_base_units)
+
+        # And we're done
+        self.unit_numer = tuple(unit_numer)
+        self.unit_denom = tuple(unit_denom)
+        self.value = amount * (numer_factor / denom_factor)
 
     def __repr__(self):
         full_unit = ' * '.join(self.unit_numer)
@@ -229,7 +243,6 @@ class Number(Value):
             return NotImplemented
 
         amount = self.value * other.value
-        # TODO cancel out units if appropriate
         numer = self.unit_numer + other.unit_numer
         denom = self.unit_denom + other.unit_denom
 
@@ -240,19 +253,10 @@ class Number(Value):
             return NotImplemented
 
         amount = self.value / other.value
-        numer = list(self.unit_numer + other.unit_denom)
-        denom = list(self.unit_denom + other.unit_numer)
-
-        # Cancel out like units
-        # TODO cancel out relatable units too
-        numer2 = []
-        for unit in numer:
-            try:
-                denom.remove(unit)
-            except ValueError:
-                numer2.append(unit)
-
-        return NumberValue(amount, unit_numer=numer2, unit_denom=denom)
+        numer = self.unit_numer + other.unit_denom
+        denom = self.unit_denom + other.unit_numer
+
+        return NumberValue(amount, unit_numer=numer, unit_denom=denom)
 
     def __add__(self, other):
         # Numbers auto-cast to strings when added to other strings