Skip to content

Decimal spec #5413

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Decimal spec #5413

wants to merge 10 commits into from

Conversation

DrusTheAxe
Copy link
Member

Spec for decimal support

@ptorr-msft
Copy link
Contributor

For comment in meeting invite around ToString -- consider leveraging ICU. It's been in the OS since RS5 or something and is the preferred cross-platform way of doing i18n. Not sure how well it maps to WinRT anymore; Ron or Axel would know (if they haven't forgotten LOL).

static DecimalValue FromUInt64(UInt64 value);
static DecimalValue FromSingle(Single value);
static DecimalValue FromDouble(Double value);
static DecimalValue FromString(String value); // LCID=LOCALE_INVARIANT

Choose a reason for hiding this comment

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

S

Lots of our APIs tend to use CurrentLocale for the simply-named method, and make the InvariantLocale the long-named method (e.g. C#). While I wish it were the other way around, I'm hesitant to go against this precedent.

Copy link
Member

Choose a reason for hiding this comment

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

For parsing, https://learn.microsoft.com/en-us/dotnet/api/system.decimal.tostring?view=net-9.0&redirectedfrom=MSDN#System_Decimal_ToString_System_String_ FromString has overloads for (string) and (string, IFormatProvider). That follows our usual rule of adding overload parameters after the base parameter list.

For formatting, C# goes the other way https://learn.microsoft.com/en-us/dotnet/api/system.decimal.tostring?view=net-9.0 and says that there's a four-way overload, with IFormatProvider being first.

Copy link
Member Author

Choose a reason for hiding this comment

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

RECOMMEND:

  • ToString()
  • ToStringInvariant()
  • ToStringWithLocale(string localeName)

RECOMMEND: DON'T overload ToString to avoid potential confusion by developers expecting parameter isn't a locale name

Copy link
Member Author

Choose a reason for hiding this comment

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

RECOMMEND: Ping Globalization owner about this

static DecimalValue FromUInt64(UInt64 value);
static DecimalValue FromSingle(Single value);
static DecimalValue FromDouble(Double value);
static DecimalValue FromString(String value); // LCID=LOCALE_INVARIANT
Copy link
Member

Choose a reason for hiding this comment

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

For parsing, https://learn.microsoft.com/en-us/dotnet/api/system.decimal.tostring?view=net-9.0&redirectedfrom=MSDN#System_Decimal_ToString_System_String_ FromString has overloads for (string) and (string, IFormatProvider). That follows our usual rule of adding overload parameters after the base parameter list.

For formatting, C# goes the other way https://learn.microsoft.com/en-us/dotnet/api/system.decimal.tostring?view=net-9.0 and says that there's a four-way overload, with IFormatProvider being first.

@@ -45,6 +57,13 @@ data structure may be re-expressed in each language using language-specific cons
defines the `DecimalValue` structure as the Win32 `DECIMAL` definition syntax is incompatible with
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd still suggest renaming DecimalValue to just Decimal now that the name is not used by the runtime class anymore.

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

Successfully merging this pull request may close these issues.

7 participants