May review
Nobody has claimed this yet.
- Dominant language
- HTML
- Stars
- 58
- Forks
- 12
- Avg merge
- 2h 35m
- Merged PRs (30d)
- 2
Description
Here's my review so far. I like the direction and approach. A number of problems remain with rounding which will make users believe that Amount can't be used to get reliably rounded results. Also, parts of the spec contradict itself and the explainer, and I'm not sure what the intent there is.
- My main observation from last meeting still stands: whatever behavior we standardize in the first released version should be the behavior we're happy with long-term, including for things like converting 2m to person-height in the US or France. We shouldn't make users set a flag in the future to get the good result rather than the one we standardized first. Releasing this simultaneously with Intl-Sequence-Units is one possible solution.
- The explainer and spec contradict each other in the
convertToAPI. The explainer hasunitandusageoptions, the spec does not. - The spec's note in the
Amountconstructor is incorrect: The spec always stores Number and BigInt arguments directly in [[AmountValue]], regardless of the rounding options. - Exponents of 0 are not preserved when handing a string such as "0e-4". I'm happy with that behavior (and wouldn't want to change it) but wanted to make sure that's clear to everyone.
- Rounding continues to produce surprises:
- Converting 5 grams to tonnes results in 0.0000049999999999999996 tonnes instead of 0.000005 tonnes. This is caused by double rounding when doing the conversion. There are techniques to do the conversion with just a single rounding at the end which would produce the exactly rounded result as long as the conversion factors aren't too large (i.e. we're not converting solar-masses to daltons).
- Converting "0.15" meters to meters with conversion options fractionDigits=1 and the default rounding mode "halfEven" produces "1e-1" meters, which is incorrect. The correct answer is "2e-1" meters. Again, multiple rounding is the culprit here.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by comparing the explainer and spec sections for the convertTo API and the Amount constructor, then review the two rounding examples in the issue. Trace how conversion options and conversion factors are specified, and identify the intended behavior for the contradictory cases. Done means the spec and explainer agree and the rounding behavior is unambiguous.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript
- Domain
- api
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100