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

Formatting bug on ISO8601 #678

Open
dracarys18 opened this issue Apr 19, 2024 · 6 comments · May be fixed by #679
Open

Formatting bug on ISO8601 #678

dracarys18 opened this issue Apr 19, 2024 · 6 comments · May be fixed by #679
Labels
A-formatting Area: formatting A-well-known-format-description Area: well known format descriptions C-bug Category: bug in current code

Comments

@dracarys18
Copy link

Hi @jhpratt continuing our discussion from #674. I have created a repo to reproduce this bug here. You can try to run it and check it yourself.

cargo r | tee test.txt
cat test.txt | rg ":60."

This looks like this happens only when the TimePrecision is 3 here.

    const FORMAT_CONFIG: EncodedConfig = Config::DEFAULT
        .set_time_precision(TimePrecision::Second {
            decimal_digits: NonZeroU8::new(3),
        })
        .encode();

It looks like it's rounding off the time to 60 seconds and it's happening every minute. If I change the precision to 6 I can see it's adjusting to nano second

@jhpratt
Copy link
Member

jhpratt commented Apr 20, 2024

Odd. I've confirmed that the code as-is does what you say, so I'll definitely look into this more.

@jhpratt jhpratt added A-formatting Area: formatting A-well-known-format-description Area: well known format descriptions C-bug Category: bug in current code labels Apr 20, 2024
@jhpratt
Copy link
Member

jhpratt commented Apr 20, 2024

It looks like it's rounding off the time to 60 seconds and it's happening every minute.

Confirmed. This appears to be the behavior of Display for floats, and to be fair it is what 99.9% of people would expect (as it's the closest to the true value). I naïvely assumed that it would truncate, and a quick search just now reveals that the behavior isn't actually documented.

Given that I can't rely on the standard library's implementations here, I'm going to have to manually format floats. It's doable and I'll probably get to it this weekend.

@dracarys18
Copy link
Author

It looks like it's rounding off the time to 60 seconds and it's happening every minute.

Confirmed. This appears to be the behavior of Display for floats, and to be fair it is what 99.9% of people would expect (as it's the closest to the true value). I naïvely assumed that it would truncate, and a quick search just now reveals that the behavior isn't actually documented.

Given that I can't rely on the standard library's implementations here, I'm going to have to manually format floats. It's doable and I'll probably get to it this weekend.

Sure @jhpratt, I can take this up if you just give me a place where I should start looking at.

@jhpratt
Copy link
Member

jhpratt commented Apr 20, 2024

The specific function is format_float in src/formatting/mod.rs. The Some arm is what is relevant.

@dracarys18
Copy link
Author

The specific function is format_float in src/formatting/mod.rs. The Some arm is what is relevant.

Sure I will take a look and create a PR for this. Thanks!

@dracarys18
Copy link
Author

dracarys18 commented Apr 20, 2024

Hey @jhpratt, So this can be fixed by just truncating the float number to the precision point before we pass it to formatter. Playground link for it. This would require just a one line change in format_float function where we would just truncate value before it we pass it in write!. Let me know if you have better suggestions, Otherwise I will create a PR for this.

@dracarys18 dracarys18 linked a pull request Apr 20, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-formatting Area: formatting A-well-known-format-description Area: well known format descriptions C-bug Category: bug in current code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants