Skip to content

Define Decimal.Amount#188

Merged
jessealama merged 21 commits intomainfrom
decimal-amount
Apr 24, 2025
Merged

Define Decimal.Amount#188
jessealama merged 21 commits intomainfrom
decimal-amount

Conversation

@jessealama
Copy link
Copy Markdown
Collaborator

@jessealama jessealama commented Apr 3, 2025

This PR reflects current thinking about how to handle the i18n aspects of decimals.

We also clean up the README using prettier.

Comment thread spec.emu Outdated
@jessealama jessealama changed the title DefineDecimal.Amount Define Decimal.Amount Apr 4, 2025
@littledan
Copy link
Copy Markdown
Member

littledan commented Apr 9, 2025

It'd be good to accompany this proposal with a change in the README to explain the motivation, including the use cases. I don't understand where Decimal.Amount would be important to use, where the use case isn't subsumed by the Measure proposal.

If we do include decimals with precision, should we adopt the IEEE 754-2008 data model, where the equivalent of precision can go positive as well as negative?

@littledan littledan closed this Apr 9, 2025
@littledan littledan reopened this Apr 9, 2025
Comment thread spec.emu Outdated
Comment thread spec.emu Outdated
@jessealama
Copy link
Copy Markdown
Collaborator Author

If we do include decimals with precision, should we adopt the IEEE 754-2008 data model, where the equivalent of precision can go positive as well as negative?

The thinking is that the notions of precision here would be either number of significant digits or number of fractional digits. The 754 notion of positive precision wouldn't be supported, even if, under the hood, an implementation of Decimal.Amount might actually store a Decimal128 value with a positive quantum.

@littledan
Copy link
Copy Markdown
Member

It'd be good to accompany this proposal with a change in the README to explain the motivation, including the use cases. I don't understand where Decimal.Amount would be important to use, where the use case isn't subsumed by the Measure proposal.

Thinking about this more: it's somewhat cleaner to have this type separate from Measure, so that we don't have this null unit case. This is similar to how we have types like Temporal.PlainYearMonth rather than referring to the first of the month -- you want to know what data you're actually working with.

So, I support this proposal, it seems like a good, minimal addition.

@jessealama jessealama marked this pull request as ready for review April 16, 2025 03:52
@lil5
Copy link
Copy Markdown

lil5 commented Apr 16, 2025

Could we please fix the formatting first? reviewing this is not fun

Prettier e.g. has a great markdown formatter

@jessealama
Copy link
Copy Markdown
Collaborator Author

Could we please fix the formatting first? reviewing this is not fun

Prettier e.g. has a great markdown formatter

Thanks for taking a look! I do have prettier and it doesn't have anything to say about the Markdown. Do you mean that the lines are too long?

@lil5
Copy link
Copy Markdown

lil5 commented Apr 17, 2025

My grip is that the formatting should be added first as a separate PR, that way we can review the actual changes here

Yes prettier can format md files:

npx prettier -w **/*.md

@jessealama jessealama merged commit 46f6667 into main Apr 24, 2025
1 check passed
@jessealama jessealama deleted the decimal-amount branch April 24, 2025 13:44
jessealama added a commit that referenced this pull request Jul 3, 2025
* Define `Decimal128.Amount`

* Say "Decimal" rather than "Decimal128" and name section IDs consistently with `sec-decimal-*`

* Use prettier

* Add discussion of `Decimal.Amount` to the README
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.

4 participants