GeezerVison

Verdict: 1 High · 2 Medium · 1 Nit

Fixes the swapped DECIMAL(scale,precision) bug correctly, but there's one high concern: callers who followed the old (swapped) docstring, including examples/parameters.py, are now silently read the other way around and get a different type. The new widening logic also has no unit coverage. Nit: the docstring at tests/e2e/test_kernel_backend.py:690-698 still describes the explicit-arg bug as pre-existing and out of scope, and should be updated now that it's fixed.

Other findings

• ⚪ Nit — The except AttributeError fallback means a non-Decimal value (e.g. a str like "123.4567" or a float, which the constructor doesn't reject) skips widening without any signal and is sent with the under-declared scale. The value's digits would then be truncated, which is exactly what this PR sets out to prevent. Consider coercing with decimal.Decimal(str(self.value)) before calling as_tuple() so behaviour doesn't depend on the input type.

Verdict: 1 Medium · 2 Nit

The fix is correct: the cast now sends DECIMAL(precision, scale), scale=0 is handled, and an under-declared scale is widened. I found one medium issue and two low ones. The medium one is that the new scale > precision ValueError breaks callers who followed the old documented convention and had working code. The other two are about how the 38-digit cap behaves and a docstring claim that the cap contradicts.

Other findings

• ⚪ Nit — When the widened type goes over 38 digits, the cap cuts integer digits instead of scale. That can produce a type that can't hold the value, even when a valid one exists. Example: DecimalParameter(Decimal('1'*30 + '.5'), precision=12, scale=10) gives value_int_digits=30 and effective_scale=10. That's 40 digits, capped to DECIMAL(38,10), which leaves only 28 integer digits for a 30-digit value. The server will overflow (an error in ANSI mode, possibly a silent NULL otherwise). DECIMAL(31,1) would hold the value exactly, and that's what the inference path returns.

When capping, shrink the declared scale back toward value_scale before losing integer digits. For example:
effective_scale = max(value_scale, min(effective_scale, MAX_PRECISION - int_digits)), then cap. Add a unit case for it.

• ⚪ Nit — The new docstring says the value is "never rounded or truncated to a narrower declared type." That isn't true in two cases this PR handles itself:
• When the value needs more than 38 digits, the type is capped. The new unit test expects DECIMAL(38,38) for a 40-fractional-digit value, so digits are dropped.
• For non-Decimal values with no as_tuple, e.g. 1.2345 with scale=2, the declared type is used unchanged, so the value gets truncated.

Either soften the wording ("up to the 38-digit maximum, for Decimal values") or reject these inputs explicitly.

Verdict: 1 Medium · 2 Low

The fix itself looks right: DECIMAL(precision, scale) ordering, scale=0 handling, and the legacy swap with a FutureWarning all behave correctly, and the allocation math stays within 0..38. I have one medium concern: the change also widens integer digits, which the issue didn't ask for and which silently removes the server's overflow error. Two low findings cover range validation and non-Decimal inputs.

Verdict: 1 Medium · 1 Low

Looks good overall: the fix to the DECIMAL(precision, scale) order, the scale=0 handling, the legacy-swap warning and scale widening are correct and well covered by unit and E2E tests. One medium concern: widening the integer room goes beyond what the issue asked for. Nit: the debug log message hard-codes "38" instead of using MAX_PRECISION.

Verdict: 1 Medium · 2 Low

The core fix is correct: the cast is now DECIMAL(precision, scale), scale=0 is honoured, and the widening keeps all of the value's digits, with good unit and E2E coverage. One medium issue: two new lines exceed black's 88-char default, so the CI black --check src gate should fail. There are also two low concerns: the integer-room widening goes beyond what the issue asks for, and the CHANGELOG doesn't mention the new ValueError cases.

Verdict: 1 Medium · 1 Low

Mostly good: the precision/scale order is fixed and the tests are thorough. One medium issue: callers using the old convention, which already worked, now silently stop rounding because widening runs after the legacy swap. One low issue: error messages after the swap can name the wrong argument.

Verdict: 1 Medium · 1 Low

The fix is mostly sound and well tested: DECIMAL(precision, scale) ordering, the is not None checks, and the widening logic all look right. There is one medium issue: legacy calls with precision=0 used to fall back to type inference, and now they are swapped and rounded to an integer scale. The duplicated validation blocks are also mostly unreachable.

Verdict: 2 Low

Looks good overall, with 2 low concerns. The DECIMAL(precision, scale) ordering fix, the is not None check, the legacy-swap handling and the widening logic all look right on the edge cases I traced (cap at 38, integer overflow, NaN, positive exponents). Unit and E2E coverage is thorough.

Verdict: 1 Low

Looks good, with one low concern. The precision/scale order fix, the legacy-swap handling with a FutureWarning, and the scale-widening logic are all correct, and the unit and E2E tests cover them well. The remaining issue is that existing calls passing scale=0 used to get an inferred type and now bind the declared type, with no warning and no CHANGELOG entry.

Verdict: 1 Low

Looks good, with one low-severity edge case. The fix correctly binds DECIMAL(precision, scale), honours scale=0, and widens an under-declared scale. The legacy-swap, precision=0 and scale=0 paths keep backward compatibility and have thorough unit tests. Nit: the CHANGELOG entry is one very long bullet; splitting it into sub-bullets (ordering fix, widening, new validation, deprecations) would make it easier to read.

Verdict: 1 Low

This looks good, with one low concern. The precision/scale ordering fix, the is not None handling, the input validation and the backward-compat warnings are carefully done and well covered by tests. The remaining gap is that the legacy-convention case where scale == precision now gets widened silently instead of keeping the old bound type. Nit: test_decimal_widened_type_is_logged and test_decimal_widened_scale_only_is_not_warned build the same parameter and could be merged.

✅ No issues identified by the review bot.