fix: Keep the trailing zeros of a floatstring range element - #345
Conversation
Answering "does this revert half of #342?", with the function it turns onShort answer: it reverts one axis of #342's FLOAT behaviour and fully reverts the two follow-up commits. #342's INT behaviour, its containment bug fix, and its architecture all survive. The full evidence table is now in the PR description; the four-checkout probe is repeated here for the thread.
Relative to released 0.11.6, every cell that moved is a removed leading zero — which is #342's contribution, kept. The function that decides all of itWhole thing, as of the latest push: # '02.50' -> '2.50', '007' -> '7', '000' -> '0'. The lookahead leaves the last
# digit, so '0.50' keeps the zero that is its integer part.
_REDUNDANT_LEADING_ZEROS = re.compile(r"^([+-]?)0+(?=[0-9])")
def _normalized_range_element(elem: str, to_int: bool) -> Any:
"""The value an ``<intstring>``/``<floatstring>`` range element denotes.
An ``<intstring>`` becomes an ``int``. A ``<floatstring>`` keeps its text less
redundant leading zeros, so the decimal places it was written with survive
(§7.5). Returns the element unchanged when it does not denote a number.
"""
try:
if to_int:
return int(elem) # int() already drops leading zeros
# Parsed only to check it is a number; the text is what renders. Not a
# Decimal -- re-rendering one is context-sensitive and unbounded (§7.5).
float(elem)
except ValueError:
# A resolved format string can be non-numeric. Literals are checked at
# template parse time, so carry it through rather than rejecting here.
return elem
return _REDUNDANT_LEADING_ZEROS.sub(r"\1", elem)Three things worth a reviewer's attention:
The regex leaves the last digit, via the lookahead.
The comments above were about twice this long a moment ago. The |
0d692a7 to
af89643
Compare
41bf56c to
56604dd
Compare
Review on OpenJobDescription#342 pointed out that the string form of a <FloatRangeList> element exists in order to carry the scale it was written with: a range element written '2.50' is asking for `2.50` on a command line, and a renderer handed `2.5` instead has been given a different string. It is also the only way a template can ask for a fixed number of decimal places, because a <float> literal of 2.50 is the same literal as 2.5 once parsed. OpenJobDescription#342 normalized the element to the number it denotes and threw that away. Split the two kinds of zero, which are not the same thing: - Leading zeros are not part of the number and still go. '02' on an INT range is the task value 2, and forwarding the text renders `--frame 02`. - Trailing zeros are the author's chosen scale and are kept. '02.50' renders `2.50`, not `2.5`. The already-landed conformance fixture EXPR/jobs/expr1.3.4--float-passthrough pins the same rule for a FLOAT parameter default, so both it and base/jobs/3.4.1.2 can now be right at once. openjd-specifications#180 states the rule as Template Schemas §7.5. A <floatstring> therefore renders as its own text less any redundant leading zeros, which is a substring operation rather than a numeric one. That is what removes the machinery the two preceding commits added, because every problem they were solving came from having to choose a notation in which to re-render a Decimal: - normalize() and quantize() round to getcontext().prec, so an embedding application's decimal context changed what this library rendered: measured, '1.2345678901234567890123456789012345678901' came back as '1.2346' under getcontext().prec = 5. - str(Decimal) switches to exponent notation once the adjusted exponent falls below -6, so '0.0000001' rendered 1E-7. - format(value, 'f') is exact and plain at every magnitude but unbounded in the exponent, so '1e999999999' -- 11 characters of template text -- expanded to ~10**9 characters and needed a length bound to stay safe. Text needs none of those decisions. '1e999999999' now costs its own 11 characters, so the exponent bound and its boundary cases are gone with it, and an element can no longer silently exceed TaskParameterStringValueAsJob's 1024-character cap and fall through to a numeric member of the union. An <intstring> is unaffected: int() already discards leading zeros and there are no fractional digits to preserve. Verification. Model suite 5527 -> 5529 passed, 24 skipped, 3 xfailed; ruff, black and mypy clean at the CI-pinned versions. Three mutants confirm the tests pin both halves of the rule: dropping the leading-zero strip fails 7 tests, restoring the trailing-zero strip fails 7, and dropping the int() conversion fails 3. Full 2023-09 conformance suite, run from source against openjd-cli mainline 7c7ece4 with openjd-sessions 0.10.14, on openjd-specifications#180's tree: 1162 passed, 0 failed. The baseline at e7a17b3 passes 1161 and fails only base/jobs/7.5--numeric-string-zeros-in-range-elements, so this closes that one fixture and moves nothing else. Both controls hold -- base/jobs/3.4--float-parameter and EXPR/jobs/expr1.3.4--float-passthrough. Relative to released 0.11.6 the only behaviour change here is the leading-zero trim: every element whose rendering differs from 0.11.6 differs by a removed leading zero. openjd-rs, which backs this package's `openjd.model._v1` API, needs the matching change; that is OpenJobDescription/openjd-rs#354. With both in place the two implementations render every range-element case tested identically, including the zero and exponent forms. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Two review findings, both cases where the forwarded text did not match what openjd-rs renders for the same element. float() ignores surrounding whitespace but the regex does not, so ' 1.5 ' was forwarded with its spaces and reached a command line as --frame ' 1.5 '. openjd-rs trims the resolved element before keeping it; do the same here. Zero has no sign. '-0.0' rendered -0.0 here and 0.0 on mainline and in openjd-rs, which is the reading the deleted test pinned by name. Drop the sign without dropping the decimal places, so '-0.00' renders 0.00. Text that reaches zero only by underflow, like '1e-400', does not render the value and is not kept -- which also covers the huge negative exponents, since '1e-999999999' underflows to zero. Both now agree with openjd-rs#354 on every case measured: ' 1.5 '->1.5, '-0.0'->0.0, '-0.00'->0.00, '1e-400'->0.0, '5.'->5., '.5'->.5, '+2.50'->+2.50, '1E+2'->1E+2. Also soften a test comment that claimed an element can no longer exceed TaskParameterStringValueAsJob's cap and fall through to a numeric member of the union. That holds for the huge-exponent inputs the test parametrizes, but long literal text still can, on this branch and on mainline and 0.11.6 alike -- only the expansion is gone. Verification. Model suite 5533 passed, 24 skipped, 3 xfailed; ruff, black and mypy clean. Full 2023-09 conformance 1162 passed, 0 failed. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
Review found that testing `value == 0.0` discarded the digits of any value too small for a float. '0.' followed by 400 zeros and a 1 -- plain decimal text, exactly what a <floatstring> is for -- underflowed and rendered 0.0, silently a different number. The cutoff was an artifact of binary64 rather than anything in the template: 1e-320 is subnormal and survived, 1e-400 did not. Ask the text instead. An all-zero mantissa spells zero whatever the exponent, so '0.00', '-0.0' and '0e5' lose their sign and keep their digits, while '1e-400' and '0.000...1' keep both. Mirrors openjd_expr::value::text_spells_zero, so openjd-rs#354 renders every one of these identically. Verification. Model suite 5536 passed, 24 skipped, 3 xfailed; ruff, black and mypy clean. Full 2023-09 conformance 1162 passed, 0 failed. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
56604dd to
0e24863
Compare
* chore(deps): Bump openjd-* Rust crates to the 0.6.0 release Move the bindings crate onto the openjd-rs release published by OpenJobDescription/openjd-rs#357: openjd-expr 0.5.0 -> 0.6.0 (breaking) openjd-model 0.5.4 -> 0.6.0 (breaking) openjd-sessions 0.5.4 -> 0.5.5 The breaking part of both minor bumps is openjd-rs#354, "Keep the decimal places of a floatstring range element", the Rust counterpart of #345 on this side. `TaskParameter::Float` now carries `Vec<Float64>` rather than `Vec<f64>`, because a `<floatstring>` range element has to keep the scale it was written with: '02.50' renders `2.50`, not `2.5` (Template Schemas §7.5). That reaches Python through `TaskParameterValue`, which renders the preserved spelling verbatim, so the per-task value a command line receives now matches the pure-Python reference. `FloatTaskParameter.range` stays `list[float]` -- it is the numeric introspection view of the resolved definition -- so the conversion takes `Float64::value()` there. openjd-model 0.6.0 also carries openjd-rs#355 (two chunking parity gaps) and openjd-rs#358 (the 512-character cap on a let binding identifier), and openjd-sessions 0.5.5 carries openjd-rs#361 (persist a resolved symbol table supplied by argument). Verified: cargo build --all-targets, cargo clippy --all-targets -D warnings, cargo test and cargo test --doc all pass. The Python suite is 5546 passed with 5 failures that are all gap markers this release closes; they are addressed in the following commit. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> * test: Promote the two chunking gaps openjd-model 0.6.0 closes openjd-rs#355 closed both divergences that `test/openjd/model_v1/test_known_gaps.py` recorded as strict xfails, so with `xfail_strict = true` they now fail as xpasses. That file's own rule is to promote a resolved gap to its proper home rather than drop the marker in place, so both move to `test_step_param_space_iter.py` beside the rest of `TestChunksTaskCountOverride`. - A CONTIGUOUS chunked space supports random access. `it[0]`, `it[1]` and `it[-1]` answer, indexing observes `chunks_task_count_override`, and one past the end is still an IndexError. This replaces `test_a_contiguous_space_refuses_indexing_with_or_without_the_override`, which asserted the refusal and named this exact swap as its counterpart. - `chunks_parameter_name` and `chunks_default_task_count` report for any chunked space, not only an adaptive one, which is what v0 has always done. Added `test_an_adaptive_space_still_refuses_indexing` as the negative control and the remaining limitation: an adaptive space has no knowable count, so `len()` raises ValueError and every index is out of range, while iteration still yields. Measured, along with everything asserted above, against openjd-model 0.6.0 before the assertions were written. Both promoted assertions are falsifiable by the version alone: they were strict xfails passing on mainline at openjd-model 0.5.4, so they failed there, and the CI run on 007f212 reports them xpassing at 0.6.0. `specs/python-model-interface.md` claimed both limitations and pointed at the xfails by name; it now states the random access that works and the one adaptive limitation that remains. The `Optional[str]`/`Optional[int]` signatures are unchanged -- both getters still answer None for a space that is not chunked. Verified: 5551 passed, 24 skipped, 3 xfailed with the 94% coverage gate enforced; ruff, black and mypy clean. `test_known_gaps.py` is now down to one test, which is a passing regression test rather than a gap. Left where it is rather than widen this change. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> * chore: Regenerate THIRD-PARTY-LICENSES for the openjd-* 0.6.0 bump `scripts/check_third_party_licenses.sh` fails on a Cargo.lock change alone, and the three crate bumps are exactly what the diff contains: openjd-expr 0.5.0 -> 0.6.0, openjd-model 0.5.4 -> 0.6.0, openjd-sessions 0.5.4 -> 0.5.5. No other line moves, and no transitive dependency changed. Regenerating it needed a portability fix first. `sed -i 's/\r//'` on the EOL normalization line is GNU-only: BSD sed reads the next argument as the backup suffix, so on macOS the script died with `sed: 1: "/var/folders/...": invalid command code f` before writing anything. Rewriting through a temp file behaves identically on both. The failure was not specific to this change -- the script could not be run on macOS at all -- and CI regenerates with the same script, so the committed file and the check stay in agreement. Verified: `scripts/check_third_party_licenses.sh --update` then `scripts/check_third_party_licenses.sh` reports the file up to date, with cargo-about 0.9.2, the version CI installs. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --------- Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
What
The string form of a
<FloatRangeList>element keeps the trailing zeros it was written with. Leading zeros still go.Why
Review on #342 made the point that the string form exists in order to carry the scale it was written with. A range element written
'2.50'is asking for2.50on a command line; a renderer handed2.5has been given a different string. #342 normalized the element to the number it denotes and threw that away.The two kinds of zero are not the same thing, and this splits them:
'02'on an INT range is the task value2; forwarding the text renders--frame 02'02.50'renders2.50This is also what lets two conformance fixtures both be right at once. The already-landed
EXPR/jobs/expr1.3.4--float-passthroughassertsPARAM:3.500from a FLOATdefault: "3.500".base/jobs/3.4.1.2--float-range-floatstring-elements-normalized(in openjd-specifications#179) was written to state the opposite reading for a range element, and its own comment flagged that the two could not coexist and that a spec ruling was needed. The ruling is trailing-keep, so the range-element fixture is the one that moves.What this deletes, and why
A
<floatstring>now renders as its own text less any redundant leading zeros. That is a substring operation, not a numeric one, which removes the machinery d6d5540 and e7a17b3 added — every problem those commits were solving came from having to pick a notation in which to re-render aDecimal:normalize()andquantize()round togetcontext().prec, so an embedding application's decimal context changed what this library rendered.str(Decimal)switches to exponent notation below1e-6, so'0.0000001'rendered1E-7.format(value, 'f')is exact and plain at every magnitude but unbounded in the exponent, so'1e999999999'— 11 characters of template text — expanded to ~109 characters and needed a length bound to stay safe.Text needs none of those decisions.
'1e999999999'now costs its own 11 characters, so the exponent bound and its four boundary cases go with it, and an element can no longer silently exceedTaskParameterStringValueAsJob's 1024-character cap and fall through to a numeric member of the union. Net 68 insertions, 153 deletions, one commit.An
<intstring>is unaffected:int()already discards leading zeros, and there are no fractional digits to preserve.Verification
Model suite 5527 → 5529 passed, 24 skipped, 3 xfailed.
ruff,black,mypyclean at the CI-pinned versions.Conformance, full
2023-09/*suite (1162 fixtures) run from source —openjd-cliat mainline7c7ece4,openjd-sessionsat0.10.14, which is the newest combination the CLI supports — against openjd-specifications#180's tree:e7a17b3(mainline)base/jobs/7.5--numeric-string-zeros-in-range-elementsThe one fixture that pins this rule goes green and nothing else moves. That includes both controls:
base/jobs/3.4--float-parameter(a<float>numeric literal rendering1.0) andEXPR/jobs/expr1.3.4--float-passthrough(a FLOATdefault: "3.500"rendering3.500) both pass.Mutation-checked. Three mutants confirm the tests pin both halves of the rule rather than just asserting the current output:
int()conversion removedCross-implementation agreement
openjd-rs, which also backs this package's
openjd.model._v1API, needed the matching change: it stored a resolved FLOAT range asVec<f64>, so'02.50'rendered2.5. That is OpenJobDescription/openjd-rs#354. With both branches in place the two implementations render every range-element case tested identically:'1.5''02.50''3.500''007'1.52.503.5007'0.50''000''0.00'0.5000.00'1E+2''1e-3''+2.50'1E+21e-3+2.50Before the two changes they disagreed on eight of those ten.
Review round
Five findings, all measured against 0.11.6, mainline
e7a17b3, and this branch before acting. Two were correct and are fixed in0708da8; three do not hold.float()forwards' 1.5 'with its spaces1.5; openjd-rs trims the resolved element, so this does too. The underscore case ('1_0.5') is real but pre-existing — released 0.11.6 renders it the same way and openjd-rs rejects the template outright, so it is a template-layer parity issue, not this change's.-0.0renders-0.0, diverging from openjd-rs-0.0→0.0and-0.00→0.00. Text that reaches zero only by underflow (1e-400) no longer renders a string that disagrees with the value. The+case needed no change: openjd-rs renders+2.50too, measured.float1.2345678901234567). The old bound ran before the strip, so it returned the element unchanged at 1126 characters — the strip was never reached. Pre-existing since 0.11.6. The comment that over-claimed a guarantee was reworded.'007'→7reintroduces a divergence9a9998drendered7.0, openjd-rs#354 renders7. The suggested fix (append.0when no point) would create the divergence.1E+2, and so does openjd-rs on both sides of its fix.100.0existed only in the two unreleased commits this PR revises, and forcing plain notation is what produced the unbounded-expansion bug. §7.5 marks it unspecified.Known remaining divergence
On the FLOAT job parameter default surface, openjd-rs forwards the text verbatim and does not strip leading zeros:
default: '007'renders007there and7here, anddefault: '01.250'renders01.250there and1.250here. Template Schemas §7.5 rule 1 says they should be stripped, so openjd-rs is the side that is wrong, but no conformance fixture pins it and it is a pre-existing difference on a surface this change does not touch. Left for a follow-up rather than widened into either PR.Exponent notation and an explicit leading
+also still differ between the two on the default surface ('1e-3'renders0.001here,1e-3there). §7.5 calls both out as unspecified in this revision.Related
Does this revert half of #342?
Asked in review. No — it reverts one axis of #342's FLOAT behaviour, and fully reverts the two follow-up commits, but #342's INT behaviour, its bug fix, and its architecture all survive.
Rendered range-element values, probed against four checkouts, each built and run rather than read:
f5da1b0e7a17b3'02'02222'003'003333'1.5'1.51.51.51.5'02.50'02.502.52.52.50'3.500'3.5003.53.53.500'007'00777.07'0.50'0.500.50.50.50'0.00'0.0000.00.00#342 collapsed a
<floatstring>to the number it denotes, which dropped leading and trailing zeros in one action. Those are separable, and only the second is reverted.What survives from #342
'02'→2is fix: Normalize intstring/floatstring task parameter range elements #342's behaviour, untouched.'02.50'→2.50still loses the0;'007'→7.git diff f5da1b0..HEAD -- src/openjd/model/_step_param_space_iter.pyis empty. Therange_set={str(v) for v in parameter.range}repair and the test that pins it are exactly as fix: Normalize intstring/floatstring task parameter range elements #342 left them._normalize_numeric_range_elementsis still amode="before"validator onRangeListTaskParameterDefinition, in the same place, keying off the validatedtype, so it still covers all three inbound range paths.What is reverted in full is
d6d5540ande7a17b3, not #342. Those existed only to make re-rendering aDecimalsafe: context-sensitive rounding, exponent notation below1e-6, and the unboundedformat(v, 'f')expansion that needed a 1024-character bound. Forwarding text needs none of it. That is where the −132 lines come from, and it is also why'007'renders7rather than mainline's7.0— the "always keep one fractional digit" rule was a follow-up invention, not #342's.Proportions, from the diffs
Source only, excluding tests:
v2023_09/_model.py).The sharpest statement: relative to the last release, the only behaviour change left on this branch is the leading-zero trim. Every cell where this branch differs from 0.11.6 is a removed leading zero. That trim is #342's contribution, kept. So #342 nets out as retained-and-narrowed rather than half-reverted; the machinery being reverted belongs to the commits that came after it.