fix: bump openjd-rs crate deps to 0.3.0/0.5.0/0.5.0 - #329
Conversation
| openjd-expr = "0.2.1" | ||
| openjd-model = "0.4.0" | ||
| openjd-sessions = "0.4.0" | ||
| openjd-expr = "0.3.0" |
There was a problem hiding this comment.
THIRD-PARTY-LICENSES.txt was not regenerated alongside this dependency bump, so the committed file is now stale relative to Cargo.lock.
The THIRD-PARTY-LICENSES check job in .github/workflows/rust_quality.yml:183 regenerates the file from Cargo.lock via cargo about and diffs it against the committed copy, so this will likely fail CI. The committed file still records the pre-bump versions:
2073:** ahash; version 0.8.12 (ahash was removed from Cargo.lock in this PR)
2525:** granit-parser; version 0.0.7 (now 1.0.1)
2529:** openjd-expr; version 0.2.1 (now 0.3.0)
2545:** serde-saphyr; version 0.0.29 (now 1.0.1)
Plus the many other version deltas in the Cargo.lock diff, and the crates newly pulled in / dropped (getrandom, r-efi, wasip2, which, version_check). Running scripts/check_third_party_licenses.sh --update should bring it back in sync.
| format!( | ||
| "IntRange(start={}, end={}, step={})", | ||
| self.inner.start, self.inner.end, self.inner.step | ||
| self.inner.start(), self.inner.end(), self.inner.step() |
There was a problem hiding this comment.
This line is likely not rustfmt-clean. self.inner.start(), self.inner.end(), self.inner.step() is 60 chars of args on one line inside a format! whose first arg already spans a line — with the field-access form it fit, but the added () calls push it past the width where rustfmt breaks each argument onto its own line:
format!(
"IntRange(start={}, end={}, step={})",
self.inner.start(),
self.inner.end(),
self.inner.step()
).github/workflows/rust_quality.yml:97 runs cargo fmt --check, so this would fail the quality gate. Worth running cargo fmt over the file. (Line 94, the __reduce__ tuple, may be affected too.)
| # total = 6 | ||
| assert result.operation_count == 6 | ||
| # total = 16 | ||
| assert result.operation_count == 16 |
There was a problem hiding this comment.
The 6 -> 16 jump here looks anomalous and worth verifying against the upstream change rather than just accepting the new number.
The other two updated expectations move by exactly +1 (join 4→5, zfill 6→7), consistent with "more granular budgeting" adding one op somewhere. But repr_sh on a 500-char string jumps by +10, while:
repr_sh(hello)(line 256) is unchanged at 2, so the base cost ofrepr_shdid not change.re_search(a * 500, ...)(line 105) is unchanged at 6, so__mul__on a 500-char string still costs 3 and a peer string function on the same 500-char input still costs 3.
So under the documented 1 + ceil(len/256) model this should still be 6, and nothing else in the file corroborates a 10x-ish per-chunk cost that only affects repr_sh at length 500 but not at length 5. That pattern is more consistent with an upstream shell-quoting path being charged per-character (or per some smaller chunk) than with an intentional budgeting change — which would matter for users, since repr_sh on large strings would now burn the operation budget ~5x faster than re_escape/zfill on the same input.
Also, the inline comments above all three changed asserts still show the old arithmetic and now contradict the asserted values:
- line 110-111 computes
3 + 3but asserts 16 - line 276 says
1 call + 3 list iterations = 4but asserts 5 - line 283-284 computes
3 + 3but asserts 7
Please update the comments to explain the new arithmetic (they are the only documentation of the counting model in this file), and confirm the 16 is intended upstream behavior.
There was a problem hiding this comment.
hold on. updating the comment, but it seems like upstream openjdrs changed the way this is counted.
| @@ -12,9 +12,9 @@ name = "_openjd_rs" | |||
| crate-type = ["cdylib", "rlib"] | |||
|
|
|||
There was a problem hiding this comment.
The stated goal of this PR may not actually be achieved by the committed lockfile.
The commit message says:
openjd-model 0.4.0 -> 0.5.0 (resolves 0.5.1 via semver)
openjd-sessions 0.4.0 -> 0.5.0 (resolves 0.5.1 via semver)
But Cargo.lock pins 0.5.0 for both, not 0.5.1:
Cargo.lock:698 name = "openjd-model" version = "0.5.0"
Cargo.lock:730 name = "openjd-sessions" version = "0.5.0"
Since Cargo.lock is committed and CI builds use it, the "wrap-action bug fixes" this PR is meant to pick up will not be in the built artifact if they landed in 0.5.1. A cargo update -p openjd-model -p openjd-sessions (or bumping the manifest requirement to "0.5.1" to make the floor explicit) would be needed.
Worth double-checking which patch version actually contains the wrap-action fix, and whether the lockfile reflects it.
52e02b6 to
c12c05a
Compare
3338eb0 to
99aab32
Compare
Bump Rust crate dependencies to pick up wrap-action bug fixes: - openjd-expr 0.2.1 -> 0.3.0 - openjd-model 0.4.0 -> 0.5.0 (resolves 0.5.1 via semver) - openjd-sessions 0.4.0 -> 0.5.0 (resolves 0.5.1 via semver) Adapts range_expr.rs for expr 0.3.0 breaking changes: - IntRange fields are now private (use accessor methods) - RangeExpr::from_values returns Result Updates 3 string-operation-counting test expectations to match expr 0.3.0's more granular operation budgeting. Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
99aab32 to
4d620c7
Compare
| # total = 6 | ||
| assert result.operation_count == 6 | ||
| # zfill: charges on the output length (500) | ||
| # 1 + ceil(500/256)=2 + 1 dispatch = 4 |
There was a problem hiding this comment.
The updated comments document a counting model that the rest of this file contradicts, and the module docstring was not updated alongside them.
I checked the repr_sh derivation and it is self-consistent — escaped_bound(5) = 5*6+2 = 32, 1 + ceil(32/256) = 2, matching the unchanged repr_sh('hello') == 2 at line 258. So the 16 looks right. The issues are the surrounding documentation and the other two expectations.
1. The module docstring (lines 3-8) still states the old universal rule:
When a function processes a string or path value, ceil(len/256) is added
to the operation count.
That is now false for repr_sh (charges ceil(escaped_bound(len)/256), ~6x the input length) and for zfill (charges on the output length plus an extra term). Since this docstring is the only statement of the counting contract in the file, it should describe the actual rules — or at least note that some functions charge a worst-case-output bound rather than input length.
2. zfill's + 1 dispatch term (line 286) is inconsistent with peer functions.
No other string method here is charged a dispatch op: ('abc' * 100).replace('a','x') is 3 + 3 = 6 (line 64), ('a' * 1000).upper() is 5 + 5 = 10 (line 56), and the parametrized cases at lines 236-266 are all 1 + ceil(len/256) with no extra term. If zfill genuinely costs one more op than replace for comparable work, that is either an upstream inconsistency worth reporting to openjd-rs, or the + 1 accounts for something else (e.g. the padding write charged separately) and the comment mislabels it. As written it reads as a number fitted to the observation rather than a derivation.
3. join's expectation is now purely observational (line 277: Observed total from openjd-rs 0.3.0 = 5). That makes the assertion a change-detector: it will flag future upstream drift but gives a maintainer no basis to judge whether a new number is a bug or intended. Deriving the 5 explicitly (1 call + 3 list iterations + 1 for the 5-char result?) would keep it a conformance check.
Bump Rust crate dependencies to pick up wrap-action bug fixes:
Adapts range_expr.rs for expr 0.3.0 breaking changes:
Updates 3 string-operation-counting test expectations to match expr 0.3.0's more granular operation budgeting.
Fixes:
What was the problem/requirement? (What/Why)
What was the solution? (How)
What is the impact of this change?
How was this change tested?
See DEVELOPMENT.md for information on running tests.
Was this change documented?
Is this a breaking change?
A breaking change is one that modifies a public contract in a way that is not backwards compatible. See the
Public Interfaces section
of the DEVELOPMENT.md for more information on the public contracts.
If so, then please describe the changes that users of this package must make to update their scripts, or Python applications.
Does this change impact security?
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.