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

Format doesn't properly handle edge cases for the %f verb #85

Open
OpinionatedGeek opened this issue Mar 7, 2018 · 12 comments
Open

Format doesn't properly handle edge cases for the %f verb #85

OpinionatedGeek opened this issue Mar 7, 2018 · 12 comments
Assignees
Labels

Comments

@OpinionatedGeek
Copy link

I don't know if you're interested in these formatting problems or not. I promise you I'm not deliberately seeking them!

From my logs it looks as if a lot of small numbers are being formatted as 0. I can reproduce it with the value 0.01 (I use 1% often in some code) and the format string '%.10f' (which I pretty much use for all the values I print).

I've updated my number format program to show you exactly what I mean:

package main

import (
	"fmt"

	"github.com/ericlagergren/decimal"
)

func testPrint(floatValue float64, decimalValue string) {
	fmt.Printf("F: %.10f\n", floatValue)
	decimal, _ := new(decimal.Big).SetString(decimalValue)
	fmt.Printf("D: %.10f\n", decimal)
}

func main() {
	testPrint(0.1234567891, "0.1234567891")
	testPrint(0.123456789, "0.123456789")
	testPrint(0.123, "0.123")
	testPrint(0.0000000000000000000000000000000000000000000000000000000000001, "0.0000000000000000000000000000000000000000000000000000000000001")
	testPrint(0.01, "0.01")

	fmt.Printf("F: %f\n", 6.0)
	testDecimal, _ := new(decimal.Big).SetString("6.0")
	fmt.Printf("D: %f\n", testDecimal)
}

The current code outputs:

F: 0.1234567891
D: 0.1234567891
F: 0.1234567890
D: 0.1234567890
F: 0.1230000000
D: 0.1230000000
F: 0.0000000000
D: 0.0000000000
F: 0.0100000000
D: 0.0000000000
F: 6.000000
D: 6.00

(The last lines show a small inconsistency with '%f' but that's less of a concern for me.)

Many thanks,

Geoff

@ericlagergren
Copy link
Owner

I appreciate the issues. I need to get %f fixed before the next release.

@OpinionatedGeek
Copy link
Author

Thanks. Also, I wondered about %d too - it would be nice (for me) if it truncated to an integer, since it (and %x, %o etc.) are integer verbs. Maybe all integer verbs (%b, %c, %d, %o, %q, %x, %X, %U) could do this and format the same way they would an int?

I've no idea if that's a good idea for anyone else though but I thought I'd mention it.

@ericlagergren
Copy link
Owner

While I'm sympathetic to the idea, I feel like that's a big—but also subtle—breaking change. For example, changing a function signature is a breaking change, but it's also noisy—the code won't compile. Silently truncating decimals is quiet and easy to miss until you realize you're charging a customer $3 instead of $3.14 and production is on fire. (Granted, that requires multiple failures along the way but still...) Even for v4 (a major version, so breaking changes are allowed) I'd be a little nervous making the change.

I'm open to using a verb other than one that's already taken by either this package or the fmt package (to reduce confusion). If you wanted to create a PR for that I'd be more than happy to consider it.

@OpinionatedGeek
Copy link
Author

Yeah, I take your point but I figured I'd mention it. I guess I just assumed fmt integer verbs would truncate to integers here, but I'm not sure why. I also thought about maybe capitalised versions truncating to int (so %D etc.) but that wouldn't work so well with the %x/%X distinction. Oh well - thanks for your thoughts.

@fantomgs
Copy link
Contributor

fantomgs commented Mar 12, 2018

#87 fix %f format issue

@ericlagergren
Copy link
Owner

Keeping this open for a bit just until I've bettered the tests.

@ericlagergren ericlagergren changed the title Formatting 0.01 to 10 decimal places just prints zeroes Format doesn't properly handle edge cases for the %f verb Mar 14, 2018
@ericlagergren ericlagergren self-assigned this Mar 24, 2018
@mpwalkerdine
Copy link

Not sure if this is the desired behaviour in the Go operating mode, but this test fails (gives 0 instead of 0.00). Leaving it in GDA mode will make it pass.

func TestDecimal_FormatGo(t *testing.T) {
	z := new(Big)
	z.Context.OperatingMode = Go
	fs := "%.2f"
	want := "0.00"
	got := fmt.Sprintf(fs, z)
	if got != want {
		t.Fatalf("printf(%s)\ngot   : %s\nwanted: %s", fs, got, want)
	}
}

@mpwalkerdine
Copy link

Another one (again, only in Go mode, gives '0.00000' instead of ' 0.00')

func TestDecimal_FormatGo(t *testing.T) {
	z := new(Big)
	z.Context.OperatingMode = Go
	fs := "'%5.2f'"
	want := "' 0.00'"
	got := fmt.Sprintf(fs, z)
	if got != want {
		t.Fatalf("printf(%s)\ngot   : %s\nwanted: %s", fs, got, want)
	}
}

@mpwalkerdine
Copy link

It looks like the above two are intentional.

	if x.compact == 0 && o == Go {
		// Go mode prints zeros different than GDA.
		if f.width == noWidth {
			f.WriteByte('0')
		} else {
			f.WriteString("0.")
			io.CopyN(f, zeroReader{}, int64(f.width))
		}
		return
	}

fmt.Sprintf("'%5.2f'", 0.) gives ' 0.00', so I'm not sure I see the rationale for this. It certainly surprised me. If it's here to stay it would be worth adding another item to the list of operating mode differences.

@ericlagergren
Copy link
Owner

@mpwalkerdine hi I just wanted to let you know I saw your comments and am taking a look. Sorry for the delay.

@mpwalkerdine
Copy link

No worries, and thanks for the great library! 😃

@ericlagergren
Copy link
Owner

Update: I’m rewriting part of the Format code because it’s too complicated right now. That’ll fix the issues.

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

No branches or pull requests

4 participants