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.

Sign up for GitHub

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

Jump to bottom

Add more functions to CompactDecimalFormatter #3699

Merged
merged 8 commits into from
Sep 21, 2023
Merged

Add more functions to CompactDecimalFormatter #3699

merged 8 commits into from
Sep 21, 2023

Conversation

sffc
Copy link
Member

@sffc sffc commented Jul 18, 2023 •
edited
Loading

Part of #3647
Depends on #3945

The concerns about double rounding were discussed in #3647. I'd like to add a function that directly formats a FixedDecimal because otherwise this class is too hard to use.

I also made the following changs:

  1. Added FormattedFixedDecimal::get_compact_decimal to get the result of the compact decimal conversion
  2. Renamed some things in the FixedDecimal crate, but left the old names as re-exports so to not break semver
  3. Added format_f64, which is fallible since f64-to-FixedDecimal is fallible
  4. Added the ryu feature to the CompactDecimalFormatter crate

Questions:

  1. Can I name the function format instead of format_fixed_decimal?

@sffc sffc requested review from echeran, eggrobin and sven-oly July 18, 2023 16:00
@sffc sffc changed the title Add format_fixed_decimal to CompactDecimalFormatter Add format_fixed_decimal to CompactDecimalFormatter and get_compact_decimal to FormattedCompactDecimal Jul 18, 2023
@sffc sffc changed the title Add format_fixed_decimal to CompactDecimalFormatter and get_compact_decimal to FormattedCompactDecimal Add CompactDecimalFormatter::format_fixed_decimal, FormattedCompactDecimal::get_compact_decimal Jul 18, 2023
@sffc sffc changed the title Add CompactDecimalFormatter::format_fixed_decimal, FormattedCompactDecimal::get_compact_decimal Add more functions to CompactDecimalFormatter Jul 18, 2023
@sffc sffc requested a review from younies July 25, 2023 15:38
younies
younies previously approved these changes Aug 7, 2023
View reviewed changes
Copy link
Member

@younies younies left a comment

Choose a reason for hiding this comment

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

Fix the failing tests.

eggrobin
eggrobin previously approved these changes Aug 7, 2023
View reviewed changes
experimental/compactdecimal/src/compactdecimal.rs
}

/// Formats an integer in compact decimal notation using the default
/// precision settings.
Copy link
Member

Choose a reason for hiding this comment

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

Do I understand correctly that if you want something fancier, e.g., at least three digits (like YT subscriber counts), or directed rounding (like everything at YT) you then need to round yourself?

(That works for me.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you be okay if we landed this PR and then in the future we added such options to CompactDecimalFormatterOptions?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, though the rounding direction discussion would then come back to haunt us :-)

As I wrote, I am also fine with just not having those options, and seeing the sort of thing that YouTube does as an advanced do-your-own-rounding scenario; since we have format_compact_decimal and CompactDecimal, it is much easier for the caller to have their own arbitrarily fancy rounding logic than it was for me with ICU a few years ago.

Copy link
Member Author

Choose a reason for hiding this comment

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

Follow-up filed: #3929

experimental/compactdecimal/src/compactdecimal.rs
) -> Result<FormattedCompactDecimal<'_>, CompactDecimalError> {
use fixed_decimal::FloatPrecision::Floating;
// NOTE: This first gets the shortest representation of the f64, which
// manifests as double rounding.
Copy link
Member

@eggrobin eggrobin Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don’t think this is actually a problem given that you do not give the user control over the rounding mode, and that the second rounding is only in the compact case: the two roundings are nearest ties to even, and the ties for the second rounding are representable unless you have really stupidly large numbers (being integers), so the first rounding will not produce a tie for the second.

The double rounding issue would manifest if you could request things such as « nearest, ties to even, exactly one sig. dec. »: then 95.0 / 100.0, which is a little below 0.95, would round to 0.95 in the first rounding, and to 1.0 in the second, whereas it ought to round to 0.9.

@eggrobin
Copy link
Member

eggrobin commented Aug 7, 2023

Scripsit Shane:

Questions:

  1. Can I name the function format instead of format_fixed_decimal?

No objections from me.

@sffc sffc added this to the 1.3 Blocking ⟨P1⟩ milestone Aug 19, 2023
@sffc sffc dismissed stale reviews from eggrobin and younies via a633485 August 26, 2023 01:20
@sffc sffc requested a review from a team as a code owner August 26, 2023 01:20
@sffc sffc mentioned this pull request Aug 26, 2023
Merged
@sffc
Copy link
Member Author

sffc commented Aug 26, 2023

This is mergeable, but I pulled the fixed_decimal API renames out into #3945.

I decided for now to keep the function named format_fixed_decimal because I think these APIs still need a little bit more work. But this is a step in the right direction.

@sffc
diplomat-gen
3789d14 
(cherry picked from commit cd7d7f9)
@dpulls
Copy link

dpulls bot commented Aug 29, 2023

🎉 All dependencies have been resolved !

@sffc sffc closed this Aug 30, 2023
@sffc sffc reopened this Aug 30, 2023
@sffc
Copy link
Member Author

sffc commented Aug 30, 2023

Re-running CI

sven-oly
sven-oly previously approved these changes Sep 20, 2023
View reviewed changes
Copy link
Contributor

@sven-oly sven-oly left a comment

Choose a reason for hiding this comment

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

Thanks for these updates!

experimental/compactdecimal/src/compactdecimal.rs
/// The result may have a fractional digit only if it is compact and its
/// significand is less than 10. Trailing fractional 0s are omitted, and
/// a sign is shown only for negative values.
///
Copy link
Contributor

Choose a reason for hiding this comment

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

This should document that the ryu feature is needed

echeran
echeran approved these changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@echeran echeran left a comment

Choose a reason for hiding this comment

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

LGTM

@Manishearth Manishearth merged commit 157aa78 into unicode-org:main Sep 21, 2023
@sffc sffc deleted the fmt_compact_fd branch September 21, 2023 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eggrobin eggrobin eggrobin left review comments

@echeran echeran echeran approved these changes

@younies younies younies left review comments

@sven-oly sven-oly sven-oly left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.3 Blocking ⟨P1⟩
Development

Successfully merging this pull request may close these issues.
6 participants
@sffc @eggrobin @echeran @younies @sven-oly @Manishearth