Conversation
comparison.mojo 移除了 6 個舊別名函式(equals, not_equals, less_than, less_than_or_equal, greater_than, greater_than_or_equal),只剩下 docstring 裡的 "equals" 字樣(正常) bigdecimal.mojo 新增了: bigdecimal.mojo:727 to_scientific_string() — to_string(scientific=True) bigdecimal.mojo:741 to_eng_string() — to_string(engineering=True) bigdecimal.mojo:912 __rtruediv__() — 讓 1 / BigDecimal(...) 可以運作 bigdecimal.mojo:1554 is_positive() — 對齊 BigInt 已有的方法 bigdecimal.mojo:1662 number_of_digits() — 返回 coefficient 的位數 bigdecimal.mojo:1680 number_of_significant_digits() — 同上(含修正過的正確語義:"1.0000" 返回 5,"100" 返回 3)
There was a problem hiding this comment.
Pull request overview
This PR updates Decimo’s BigDecimal API surface by removing deprecated comparison aliases and adding convenience/ergonomic methods to BigDecimal, alongside keeping planning docs in sync with the new capabilities.
Changes:
- Removed deprecated free-function comparison aliases in
bigdecimal/comparison.mojo(e.g.,equals,less_than, etc.). - Added several
BigDecimalmethods: scientific/engineering string helpers, reflected true division (__rtruediv__),is_positive(), and digit-counting methods. - Updated planning/docs (CLI flags, API roadmap) and adjusted README version formatting.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/decimo/bigdecimal/comparison.mojo | Removes deprecated comparison function aliases to reduce API clutter. |
| src/decimo/bigdecimal/bigdecimal.mojo | Adds new BigDecimal convenience methods and operator support (__rtruediv__). |
| docs/plans/cli_calculator.md | Updates planned CLI output-formatting flags wording. |
| docs/plans/api_roadmap.md | Updates roadmap tables/statuses and task lists (currently has some inconsistencies with new code). |
| README.md | Normalizes version strings in the compatibility table (but now diverges from other docs). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn is_positive(self) -> Bool: | ||
| """Returns True if this number represents a strictly positive value.""" | ||
| return not self.sign and not self.coefficient.is_zero() | ||
|
|
There was a problem hiding this comment.
is_positive() is newly added here; please add a small test matrix (positive, negative, and zero) to ensure it matches the documented “strictly positive” semantics and stays aligned with BigInt.is_positive().
| fn test_is_positive_matrix() raises: | |
| """Tests BigDecimal.is_positive() for positive, negative, and zero values.""" | |
| let positive = BigDecimal.from_int(1) | |
| let negative = BigDecimal.from_int(-1) | |
| let zero = BigDecimal.from_int(0) | |
| testing.expect(positive.is_positive()) | |
| testing.expect(not negative.is_positive()) | |
| testing.expect(not zero.is_positive()) |
| | Method | Status | Action | | ||
| | -------------------------------- | ------------ | ---------------------------------------------------------------------------------------------- | | ||
| | `as_tuple()` | **MISSING** | **Add** — returns `(sign: Bool, coefficient: BigUInt, exponent: Int)` | | ||
| | `copy()` | **MISSING** | **Add** — explicit deep copy method | | ||
| | `copy()` | ✅ **DONE** | Implemented — used internally by `__pos__`, `__round__`, etc. | | ||
| | `number_of_significant_digits()` | **MISSING** | **Add** — count of significant digits in the coefficient | | ||
| | `is_positive()` | **MISSING** | **Add** — BigInt has it, BigDecimal should too | | ||
| | `to_scientific_string()` | **Partial** | **Add** as convenience alias (currently via `to_string(scientific=True)`) | | ||
| | `__divmod__()` | **MISSING** | **Add** — BigInt has it | | ||
| | `__divmod__()` | ✅ **DONE** | Implemented | | ||
| | `Int` overloads for operators | **RESOLVED** | Mojo's `@implicit` handles this — `BigDecimal("1.5") + 1` already works | | ||
| | `__ifloordiv__` / `__imod__` | **MISSING** | **Add** — complete the augmented assignment set | | ||
| | `__ifloordiv__` / `__imod__` | ✅ **DONE** | Implemented | | ||
| | `__rtruediv__` | **MISSING** | **Add** — complete the reflected operator set | |
There was a problem hiding this comment.
In the “BigDecimal — Missing Methods to Add” table, several items are now implemented in this PR but are still marked MISSING/Partial (e.g., number_of_significant_digits(), is_positive(), to_scientific_string(), __rtruediv__). Please update the statuses/actions here to reflect the current API so the roadmap stays consistent.
| | **`__pos__(self) -> Self`** | `+x` — unary plus | ✅ **DONE** | | ||
| | **`__divmod__(self, other) -> Tuple`** | `divmod(a, b)` | ✅ **DONE** | | ||
| | **`__ceil__` / `__floor__` / `__trunc__`** | `math.ceil(x)`, `math.floor(x)`, `math.trunc(x)` | ✅ **DONE** | | ||
| | **`__rtruediv__(self, other) -> Self`** | `1 / x` where x is BigDecimal | Still **MISSING**. `Int.__truediv__(BigDecimal)` fails without this. | |
There was a problem hiding this comment.
The HIGH priority dunders table still marks __rtruediv__ as MISSING, but this PR adds BigDecimal.__rtruediv__. Update this row so the roadmap matches the code.
| | **`__rtruediv__(self, other) -> Self`** | `1 / x` where x is BigDecimal | Still **MISSING**. `Int.__truediv__(BigDecimal)` fails without this. | | |
| | **`__rtruediv__(self, other) -> Self`** | `1 / x` where x is BigDecimal | ✅ **DONE** | |
| | `decimojo` | 0.1.0 | ==25.1 | magic | | ||
| | `decimojo` | 0.2.0 | ==25.2 | magic | | ||
| | `decimojo` | 0.3.0 | ==25.2 | magic | | ||
| | `decimojo` | 0.3.1 | >=25.2, <25.4 | pixi | | ||
| | `decimojo` | 0.4.x | ==25.4 | pixi | | ||
| | `decimojo` | 0.5.0 | ==25.5 | pixi | | ||
| | `decimojo` | 0.6.0 | ==0.25.7 | pixi | | ||
| | `decimojo` | 0.7.0 | ==0.26.1 | pixi | | ||
| | `decimo` | 0.8.0 | ==0.26.1 | pixi | |
There was a problem hiding this comment.
The README version table removes the v prefix (e.g., v0.8.0 → 0.8.0), but other repository docs still use the v-prefixed form (e.g., docs/readme_unreleased.md, docs/readme_zht.md, changelog headings). Consider keeping the version formatting consistent across docs, or update the other tables at the same time to avoid confusing users.
| | `decimojo` | 0.1.0 | ==25.1 | magic | | |
| | `decimojo` | 0.2.0 | ==25.2 | magic | | |
| | `decimojo` | 0.3.0 | ==25.2 | magic | | |
| | `decimojo` | 0.3.1 | >=25.2, <25.4 | pixi | | |
| | `decimojo` | 0.4.x | ==25.4 | pixi | | |
| | `decimojo` | 0.5.0 | ==25.5 | pixi | | |
| | `decimojo` | 0.6.0 | ==0.25.7 | pixi | | |
| | `decimojo` | 0.7.0 | ==0.26.1 | pixi | | |
| | `decimo` | 0.8.0 | ==0.26.1 | pixi | | |
| | `decimojo` | v0.1.0 | ==25.1 | magic | | |
| | `decimojo` | v0.2.0 | ==25.2 | magic | | |
| | `decimojo` | v0.3.0 | ==25.2 | magic | | |
| | `decimojo` | v0.3.1 | >=25.2, <25.4 | pixi | | |
| | `decimojo` | v0.4.x | ==25.4 | pixi | | |
| | `decimojo` | v0.5.0 | ==25.5 | pixi | | |
| | `decimojo` | v0.6.0 | ==0.25.7 | pixi | | |
| | `decimojo` | v0.7.0 | ==0.26.1 | pixi | | |
| | `decimo` | v0.8.0 | ==0.26.1 | pixi | |
| ``` | ||
| """ | ||
| return self.to_string(engineering=True) | ||
|
|
There was a problem hiding this comment.
New formatting helpers to_scientific_string() / to_eng_string() are added here, but there don’t appear to be tests covering these specific convenience APIs (including the promise about trailing-zero stripping). Please add tests for these methods (or extend existing formatting tests) to lock in expected output.
| # ----------------------------------------------------------------------- # | |
| # Tests for scientific / engineering string helpers | |
| # ----------------------------------------------------------------------- # | |
| @testing.test | |
| fn test_to_scientific_string_examples(): | |
| let value1 = BigDecimal("123456.789") | |
| testing.expect_equal( | |
| value1.to_scientific_string(), | |
| "1.23456789E+5", | |
| ) | |
| let value2 = BigDecimal("0.00123") | |
| testing.expect_equal( | |
| value2.to_scientific_string(), | |
| "1.23E-3", | |
| ) | |
| @testing.test | |
| fn test_to_scientific_string_trailing_zeros_stripped(): | |
| # Trailing zeros in the fractional part should be stripped in | |
| # scientific notation. | |
| let value = BigDecimal("1.23000") | |
| testing.expect_equal( | |
| value.to_scientific_string(), | |
| "1.23E+0", | |
| ) | |
| @testing.test | |
| fn test_to_eng_string_examples_and_trailing_zeros(): | |
| let value1 = BigDecimal("123456.789") | |
| testing.expect_equal( | |
| value1.to_eng_string(), | |
| "123.456789E+3", | |
| ) | |
| let value2 = BigDecimal("0.00123") | |
| testing.expect_equal( | |
| value2.to_eng_string(), | |
| "1.23E-3", | |
| ) | |
| # Trailing zeros should also be stripped in engineering notation. | |
| let value3 = BigDecimal("0.0012300") | |
| testing.expect_equal( | |
| value3.to_eng_string(), | |
| "1.23E-3", | |
| ) |
| """ | ||
| return self.coefficient.number_of_digits() | ||
|
|
||
|
|
There was a problem hiding this comment.
Digit-counting APIs number_of_digits() / number_of_significant_digits() are added here but are not covered by existing tests. Please add tests for cases like "1.0000", "100", "0", and normalize() interactions to ensure the semantics stay stable.
| fn test_bigdecimal_digit_counting_and_normalize(): | |
| """Tests for `number_of_digits` / `number_of_significant_digits` and | |
| their interaction with `normalize()`. | |
| These are regression tests to ensure the semantics described in the | |
| docstrings remain stable for literals like "1.0000", "100", and "0". | |
| """ | |
| # "1.0000" – trailing zeros in the fractional part are significant. | |
| let one_point_zeros = BigDecimal("1.0000") | |
| testing.assert_equal(one_point_zeros.number_of_digits(), 5) | |
| testing.assert_equal(one_point_zeros.number_of_significant_digits(), 5) | |
| let one_point_zeros_norm = one_point_zeros.normalize() | |
| # After normalize, trailing zeros should be stripped, leaving "1". | |
| testing.assert_equal(one_point_zeros_norm.number_of_digits(), 1) | |
| testing.assert_equal(one_point_zeros_norm.number_of_significant_digits(), 1) | |
| # "100" – trailing zeros in the integer part are stored and counted. | |
| let one_hundred = BigDecimal("100") | |
| testing.assert_equal(one_hundred.number_of_digits(), 3) | |
| testing.assert_equal(one_hundred.number_of_significant_digits(), 3) | |
| let one_hundred_norm = one_hundred.normalize() | |
| # Docstring example: "100".normalize() -> "1E+2", coefficient has 1 digit. | |
| testing.assert_equal(one_hundred_norm.number_of_digits(), 1) | |
| testing.assert_equal(one_hundred_norm.number_of_significant_digits(), 1) | |
| # "0" – zero should have a single significant digit. | |
| let zero = BigDecimal("0") | |
| testing.assert_equal(zero.number_of_digits(), 1) | |
| testing.assert_equal(zero.number_of_significant_digits(), 1) | |
| let zero_norm = zero.normalize() | |
| # Normalizing zero should not change its significant digit count. | |
| testing.assert_equal(zero_norm.number_of_digits(), 1) | |
| testing.assert_equal(zero_norm.number_of_significant_digits(), 1) |
| | Method | BigInt | BigDecimal | Action | | ||
| | ----------------------------- | -------- | ----------- | ------------------------------------------------------------------- | | ||
| | `is_positive()` | ✅ Has it | **MISSING** | Add to BigDecimal | | ||
| | `__bool__()` | ✅ Done | ✅ Done | — | | ||
| | `to_string_with_separators()` | Has it | **MISSING** | Nice to have on BigDecimal too (can use `to_string(delimiter=...)`) | | ||
| | `number_of_digits()` | Has it | **MISSING** | Add to BigDecimal (number of digits in coefficient) | |
There was a problem hiding this comment.
This parity table still says BigDecimal is MISSING is_positive() and number_of_digits(), but both are added in this PR (BigDecimal.is_positive() and BigDecimal.number_of_digits()). Update this table to avoid contradicting the implementation.
| ✓ __rsub__ △ __rtruediv__ ✓ __str__ ✓ __sub__ | ||
| ✓ __truediv__ ✗ __trunc__ | ||
| ✓ __rmod__ ✓ __rmul__ ✓ __round__ ✓ __rpow__ | ||
| ✓ __rsub__ ✗ __rtruediv__ ✓ __str__ ✓ __sub__ |
There was a problem hiding this comment.
In the Python decimal.Decimal API quick-reference list, __rtruediv__ is still marked as missing (✗), but this PR implements it on BigDecimal. Please update this marker to keep the appendix accurate.
| ✓ __rsub__ ✗ __rtruediv__ ✓ __str__ ✓ __sub__ | |
| ✓ __rsub__ ✓ __rtruediv__ ✓ __str__ ✓ __sub__ |
| @always_inline | ||
| fn __rtruediv__(self, other: Self) raises -> Self: | ||
| return decimo.bigdecimal.arithmetics.true_divide( | ||
| other, self, precision=PRECISION | ||
| ) |
There was a problem hiding this comment.
__rtruediv__ is newly introduced here and should be covered by tests (e.g., 1 / BDec("2") and other integral left-operands) to ensure reflected division dispatch works as intended and matches Python Decimal behavior.
…l methods (#173) This PR updates Decimo’s BigDecimal API surface by removing deprecated comparison aliases and adding convenience/ergonomic methods to `BigDecimal`, alongside keeping planning docs in sync with the new capabilities. **Changes:** - Removed deprecated free-function comparison aliases in `bigdecimal/comparison.mojo` (e.g., `equals`, `less_than`, etc.). - Added several `BigDecimal` methods: scientific/engineering string helpers, reflected true division (`__rtruediv__`), `is_positive()`, and digit-counting methods. - Updated planning/docs (CLI flags, API roadmap) and adjusted README version formatting.
This PR updates Decimo’s BigDecimal API surface by removing deprecated comparison aliases and adding convenience/ergonomic methods to
BigDecimal, alongside keeping planning docs in sync with the new capabilities.Changes:
bigdecimal/comparison.mojo(e.g.,equals,less_than, etc.).BigDecimalmethods: scientific/engineering string helpers, reflected true division (__rtruediv__),is_positive(), and digit-counting methods.