Skip to content

Conversation

@JohnAD
Copy link
Contributor

@JohnAD JohnAD commented Dec 9, 2024

This is a near-duplicate of #296 that preserves the @vasilbeckk 's work and commits. I will now start making the changes requested in #296.

@JohnAD JohnAD marked this pull request as draft December 9, 2024 00:57
@JohnAD
Copy link
Contributor Author

JohnAD commented Dec 9, 2024

I added the following tests for handling trailing zeros on negative precision. Such as "1000 to the nearest 100 should serialize as 1.0E2"

{"1.0000E3", "1000.0"}, // 1000 to the nearest tenth
{"1.000E3", "1000"},    // 1000 to the nearest one
{"1.0E3", "1.0E3"},     // 1000 to the nearest hundred
{"1E3", "1E3"},         // 1000 to the nearest thousand

(for those not familiar with scientific notation usage, it is required when precision is non-fractional. For example, the population of the U.S. is 314.9 million or, more properly 3.149E8. Any digit after the 9 is an unknown.)

This test does not pass the last two. I'll have to edit some code to fix the bug.

Given that zeros are not always trailing. Perhaps the TrimTrailingZeros option flag should be generically called IgnorePrecision or something like that?

@JohnAD
Copy link
Contributor Author

JohnAD commented Dec 9, 2024

Sadly, it appears that covering both cases breaks backwards compatibility. So, I've separated it out. I've added a AvoidScientificNotation option variable. It defaults to true and that default keeps the behavior the same.

As a bonus, I went ahead and made d.ScientificNotationString() a public function and added a unit test.

@JohnAD JohnAD marked this pull request as ready for review December 9, 2024 04:49
@JohnAD JohnAD changed the title WIP: String Trim Trailing Zeros bool variable String Trim Trailing Zeros bool variable Dec 9, 2024
@JohnAD JohnAD changed the title String Trim Trailing Zeros bool variable TrimTrailingZeros and AvoidScientificNotation bool variables Dec 10, 2024
Copy link
Member

@mwoss mwoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks solid. I won't block merging it. We could fix this variable name later if needed.

Sorry for late review 🙏

//
// For example, 1200 rounded to the nearest 100 cannot accurately be shown as "1200" because the last two
// digits are unknown. With this set to false, that number would be expressed as "1.2E3" instead.
var AvoidScientificNotation = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using negative variable names often leads to double negatives in logic (e.g., if (!AvoidScientificNotation)), which can be confusing. Could we use a positive name like UseScientificNotation, WithScientificNotation? jut to decrease congnitive load.

What do you think?

@mwoss mwoss merged commit e1e92bf into shopspring:master Dec 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants