remove 'i' and nasty rounding; test

pkg/api/resource/quantity.go

@@ -41,22 +41,20 @@ import (
 // <sign>            ::= "+" | "-"
 // <signedNumber>    ::= <number> | <sign><number>
 // <suffix>          ::= <binarySI> | <decimalExponent> | <decimalSI>
-// <binarySI>        ::= i | Ki | Mi | Gi | Ti | Pi | Ei
+// <binarySI>        ::= Ki | Mi | Gi | Ti | Pi | Ei
 //   (International System of units; See: http://physics.nist.gov/cuu/Units/binary.html)
 // <decimalSI>       ::= m | "" | k | M | G | T | P | E
 //   (Note that 1024 = 1Ki but 1000 = 1k; I didn't choose the capitalization.)
 // <decimalExponent> ::= "e" <signedNumber> | "E" <signedNumber>
 //
 // No matter which of the three exponent forms is used, no quantity may represent
-// a number greater than 2^63-1 in magnitude, nor may it have more than 3 digits
-// of precision. Numbers larger or more precise will be capped or rounded.
+// a number greater than 2^63-1 in magnitude, nor may it have more than 3 decimal
+// places. Numbers larger or more precise will be capped or rounded up.
 // (E.g.: 0.1m will rounded up to 1m.)
 // This may be extended in the future if we require larger or smaller quantities.
 //
 // When a Quantity is parsed from a string, it will remember the type of suffix
 // it had, and will use the same type again when it is serialized.
-// One exception: numbers with a Binary SI suffix less than bigOne will be changed
-// to Decimal SI suffix. E.g., .5i becomes 500m. [NOT 512m!]
 //
 // Before serializing, Quantity will be put in "canonical form".
 // This means that Exponent/suffix will be adjusted up or down (with a
@@ -137,10 +135,12 @@ var (
 	big1024     = big.NewInt(1024)
 
 	// Commonly needed inf.Dec values-- treat as read only!
-	decZero     = inf.NewDec(0, 0)
-	decOne      = inf.NewDec(1, 0)
-	decMinusOne = inf.NewDec(-1, 0)
-	decThousand = inf.NewDec(1000, 0)
+	decZero      = inf.NewDec(0, 0)
+	decOne       = inf.NewDec(1, 0)
+	decMinusOne  = inf.NewDec(-1, 0)
+	decThousand  = inf.NewDec(1000, 0)
+	dec1024      = inf.NewDec(1024, 0)
+	decMinus1024 = inf.NewDec(-1024, 0)
 
 	// Largest (in magnitude) number allowed.
 	maxAllowed = inf.NewDec((1<<63)-1, 0) // == max int64
@@ -241,16 +241,14 @@ func (q *Quantity) Canonicalize() (string, suffix) {
 	switch format {
 	case DecimalExponent, DecimalSI:
 	case BinarySI:
-		switch q.Amount.Cmp(decZero) {
-		case 0: // exactly equal 0, that's fine
-		case 1: // greater than 0
-			if q.Amount.Cmp(decOne) < 0 {
-				// This avoids rounding and hopefully confusion, too.
-				format = DecimalSI
-			}
-		case -1:
-			if q.Amount.Cmp(decMinusOne) > 0 {
-				// This avoids rounding and hopefully confusion, too.
+		if q.Amount.Cmp(decMinus1024) > 0 && q.Amount.Cmp(dec1024) < 0 {
+			// This avoids rounding and hopefully confusion, too.
+			format = DecimalSI
+		} else {
+			tmp := &inf.Dec{}
+			tmp.Round(q.Amount, 0, inf.RoundUp)
+			if tmp.Cmp(q.Amount) != 0 {
+				// Don't lose precision-- show as DecimalSI
 				format = DecimalSI
 			}
 		}
@@ -281,16 +279,8 @@ func (q *Quantity) Canonicalize() (string, suffix) {
 	case BinarySI:
 		tmp := &inf.Dec{}
 		tmp.Round(q.Amount, 0, inf.RoundUp)
-		amount := tmp.UnscaledBig()
-		exponent := int(-q.Amount.Scale())
-		// Apply the (base-10) shift. This will lose any fractional
-		// part, which is intentional.
-		for exponent > 0 {
-			amount.Mul(amount, bigTen)
-			exponent--
-		}
 
-		amount, exponent = removeFactors(amount, big1024)
+		amount, exponent := removeFactors(tmp.UnscaledBig(), big1024)
 		suffix, _ := quantitySuffixer.construct(2, exponent*10, format)
 		number := amount.String()
 		return number, suffix

pkg/api/resource/quantity_test.go

@@ -62,8 +62,18 @@ func TestQuantityParse(t *testing.T) {
 		expect Quantity
 	}{
 		{"0", Quantity{dec(0, 0), DecimalSI}},
+		{"0m", Quantity{dec(0, 0), DecimalSI}},
+		{"0Ki", Quantity{dec(0, 0), BinarySI}},
+		{"0k", Quantity{dec(0, 0), DecimalSI}},
+		{"0Mi", Quantity{dec(0, 0), BinarySI}},
+		{"0M", Quantity{dec(0, 0), DecimalSI}},
+		{"0Gi", Quantity{dec(0, 0), BinarySI}},
+		{"0G", Quantity{dec(0, 0), DecimalSI}},
+		{"0Ti", Quantity{dec(0, 0), BinarySI}},
+		{"0T", Quantity{dec(0, 0), DecimalSI}},
+
 		// Binary suffixes
-		{"9i", Quantity{dec(9, 0), BinarySI}},
+		{"1Ki", Quantity{dec(1024, 0), BinarySI}},
 		{"8Ki", Quantity{dec(8*1024, 0), BinarySI}},
 		{"7Mi", Quantity{dec(7*1024*1024, 0), BinarySI}},
 		{"6Gi", Quantity{dec(6*1024*1024*1024, 0), BinarySI}},
@@ -153,19 +163,13 @@ func TestQuantityParse(t *testing.T) {
 		{"0.5Mi", Quantity{dec(.5*1024*1024, 0), BinarySI}},
 		{"0.05Gi", Quantity{dec(536870912, -1), BinarySI}},
 		{"0.025Ti", Quantity{dec(274877906944, -1), BinarySI}},
-		// These get rounded though
-		{"0.0001i", Quantity{dec(1, -3), DecimalSI}},
-		{"0.005i", Quantity{dec(5, -3), DecimalSI}},
-		{"0.05i", Quantity{dec(50, -3), DecimalSI}},
-		// Also, if below you expect (512, -3), you're wrong in two ways:
-		// In the sequence [1024*1024*1024, 1024*1024, 1024, ?], the last term is "1" not 1/1024.
-		// Even if it were, 500 * 1/1024 = .48828125, NOT .512
-		// I cannot recommend using this feature, it is confusing.
-		{"0.5i", Quantity{dec(500, -3), DecimalSI}},
 
 		// Things written by trolls
+		{"0.000001Ki", Quantity{dec(2, -3), DecimalSI}}, // rounds up, changes format
 		{".001", Quantity{dec(1, -3), DecimalSI}},
+		{".0001k", Quantity{dec(100, -3), DecimalSI}},
 		{"1.", Quantity{dec(1, 0), DecimalSI}},
+		{"1.G", Quantity{dec(1, 9), DecimalSI}},
 	}
 
 	for _, item := range table {
@@ -220,6 +224,9 @@ func TestQuantityParse(t *testing.T) {
 		"0.1mi",
 		"0.1am",
 		"aoeu",
+		".5i",
+		"1i",
+		"-3.01i",
 	}
 	for _, item := range invalid {
 		_, err := ParseQuantity(item)
@@ -239,7 +246,7 @@ func TestQuantityString(t *testing.T) {
 		{Quantity{dec(6*1024, 0), BinarySI}, "6Ki"},
 		{Quantity{dec(1001*1024*1024*1024, 0), BinarySI}, "1001Gi"},
 		{Quantity{dec(1024*1024*1024*1024, 0), BinarySI}, "1Ti"},
-		{Quantity{dec(5, 0), BinarySI}, "5i"},
+		{Quantity{dec(5, 0), BinarySI}, "5"},
 		{Quantity{dec(500, -3), BinarySI}, "500m"},
 		{Quantity{dec(1, 9), DecimalSI}, "1G"},
 		{Quantity{dec(1000, 6), DecimalSI}, "1G"},
@@ -253,10 +260,11 @@ func TestQuantityString(t *testing.T) {
 		{Quantity{dec(300, 6), DecimalSI}, "300M"},
 		{Quantity{dec(1, 12), DecimalSI}, "1T"},
 		{Quantity{dec(1234567, 6), DecimalSI}, "1234567M"},
-		{Quantity{dec(1234567, -3), BinarySI}, "1235i"},
+		{Quantity{dec(1234567, -3), BinarySI}, "1234567m"},
 		{Quantity{dec(3, 3), DecimalSI}, "3k"},
+		{Quantity{dec(1025, 0), BinarySI}, "1025"},
 		{Quantity{dec(0, 0), DecimalSI}, "0"},
-		{Quantity{dec(0, 0), BinarySI}, "0i"},
+		{Quantity{dec(0, 0), BinarySI}, "0"},
 		{Quantity{dec(1, 9), DecimalExponent}, "1e9"},
 		{Quantity{dec(1, -3), DecimalExponent}, "1e-3"},
 		{Quantity{dec(80, -3), DecimalExponent}, "80e-3"},
@@ -287,6 +295,44 @@ func TestQuantityString(t *testing.T) {
 	}
 }
 
+func TestQuantityParseEmit(t *testing.T) {
+	table := []struct {
+		in     string
+		expect string
+	}{
+		{"1Ki", "1Ki"},
+		{"1Mi", "1Mi"},
+		{"1Gi", "1Gi"},
+		{"1024Mi", "1Gi"},
+		{"1000M", "1G"},
+		{".000001Ki", "2m"},
+	}
+
+	for _, item := range table {
+		q, err := ParseQuantity(item.in)
+		if err != nil {
+			t.Errorf("Couldn't parse %v", item.in)
+			continue
+		}
+		if e, a := item.expect, q.String(); e != a {
+			t.Errorf("%#v: expected %v, got %v", item.in, e, a)
+		}
+	}
+	for _, item := range table {
+		q, err := ParseQuantity("-" + item.in)
+		if err != nil {
+			t.Errorf("Couldn't parse %v", item.in)
+			continue
+		}
+		if q.Amount.Cmp(decZero) == 0 {
+			continue
+		}
+		if e, a := "-"+item.expect, q.String(); e != a {
+			t.Errorf("%#v: expected %v, got %v", item.in, e, a)
+		}
+	}
+}
+
 var fuzzer = fuzz.New().Funcs(
 	func(q *Quantity, c fuzz.Continue) {
 		q.Amount = &inf.Dec{}
@@ -347,9 +393,9 @@ func TestMilliNewSet(t *testing.T) {
 		{1, DecimalSI, "1m", true},
 		{1000, DecimalSI, "1", true},
 		{1234000, DecimalSI, "1234", true},
-		{1024, BinarySI, "2i", false}, // Rounded up
+		{1024, BinarySI, "1024m", false}, // Format changes
 		{1000000, "invalidFormatDefaultsToExponent", "1e3", true},
-		{1024 * 1024, BinarySI, "1049i", false},
+		{1024 * 1024, BinarySI, "1048576m", false}, // Format changes
 	}
 
 	for _, item := range table {

pkg/api/resource/suffix.go

@@ -73,13 +73,15 @@ type suffixHandler struct {
 func newSuffixer() suffixer {
 	sh := &suffixHandler{}
 
-	sh.binSuffixes.addSuffix("i", bePair{2, 0})
 	sh.binSuffixes.addSuffix("Ki", bePair{2, 10})
 	sh.binSuffixes.addSuffix("Mi", bePair{2, 20})
 	sh.binSuffixes.addSuffix("Gi", bePair{2, 30})
 	sh.binSuffixes.addSuffix("Ti", bePair{2, 40})
 	sh.binSuffixes.addSuffix("Pi", bePair{2, 50})
 	sh.binSuffixes.addSuffix("Ei", bePair{2, 60})
+	// Don't emit an error when trying to produce
+	// a suffix for 2^0.
+	sh.decSuffixes.addSuffix("", bePair{2, 0})
 
 	sh.decSuffixes.addSuffix("m", bePair{10, -3})
 	sh.decSuffixes.addSuffix("", bePair{10, 0})