Repository navigation
fix(parse): += on lists extends instead of replacing; % accepts tuple args - #2
Conversation
|
The format/clippy job was already red on main (rustfmt drift in lib.rs/main.rs/eb_maintainer.rs plus three clippy findings in eb_maintainer.rs/eb_style.rs) — added a style commit here so the whole gate goes green. cargo fmt --check, clippy -D warnings, and the full suite pass locally. |
docutils requires it; the generated RST for eb_maintainer and package_emit failed the Sphinx build with warnings-as-errors.
Sphinx emits code literals as per-word spans, so multi-word phrases never appear contiguously in the raw HTML; stripping once also avoids a SIGPIPE under pipefail from sed | grep -q.
There was a problem hiding this comment.
Pull request overview
This PR fixes two correctness gaps in the easyconfig expression evaluator: += on list/tuple values now extends rather than replacing, and Python-style percent formatting (%) now supports tuple RHS arguments for positional substitution. It also adds regression tests and includes a few small formatting/doc and CI robustness tweaks.
Changes:
- Fix
+=handling so list/tuple values extend instead of replacing previous content. - Implement
%formatting with tuple RHS arguments and ensure conversions are applied left-to-right. - Add regression tests for the new evaluator behavior; minor formatting/doc cleanups and a CI-docs grep robustness tweak.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/qmcpack_foss_2026_1.rs | Minor assertion formatting change. |
| src/package_emit.rs | Doc comment spacing tweak. |
| src/main.rs | Import formatting only. |
| src/lib.rs | Re-export ordering/formatting change. |
| src/eb_style.rs | Simplifies string-assignment parsing logic for = vs +=. |
| src/eb_parse.rs | Core evaluator fixes for += list/tuple extension and % tuple-arg formatting; adds regression tests. |
| src/eb_maintainer.rs | Formatting/comment tweaks; minor readability refactors. |
| pixi.lock | Lockfile updated (adds virtual-packages + dev env stanza). |
| .github/workflows/ci_docs.yml | Makes CLI-doc assertions robust to Sphinx HTML span splitting by grepping tag-stripped text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (Value::Str(fmt), Value::Tuple(args)) => { | ||
| let mut out = fmt; | ||
| for arg in &args { | ||
| let s = match arg { | ||
| Value::Str(s) => s.clone(), | ||
| Value::Int(i) => i.to_string(), | ||
| other => { | ||
| return Err(self.err(format!( | ||
| "unsupported % tuple operand: {other:?}" | ||
| ))); | ||
| } | ||
| }; | ||
| out = python_percent_format_one(&out, &s); | ||
| } | ||
| Value::Str(out) | ||
| } |
There was a problem hiding this comment.
Good catch — fixed in f6bf8fc.
You're right that it was a silent partial substitution: with more operands than conversions the extra ones were dropped (the next python_percent_format_one call found no conversion and returned the string unchanged), and with too few a literal %s survived into the value. Python raises TypeError for both.
Now the tuple path counts conversions (skipping %% escapes) and errors on a mismatch, so the assignment fails and the field is skipped rather than filled with a wrong value — the same skip-don't-guess invariant the += fix restores. Added a regression test asserting homepage == None for both shapes (previously they yielded .../demo and .../demo/%s), plus unit coverage for the %% escape counting.
…ormat Python raises TypeError when the operand count does not match the conversion count, but the tuple path silently tolerated both shapes: extra operands were dropped (the second call found no conversion left and returned the string unchanged) and too few left a literal %s in the value. Either way a wrong value reached the field instead of the field being skipped. Counts conversions (skipping %% escapes) and errors on a mismatch, so the assignment fails and the field is omitted — the same skip-don't-guess invariant the += fix restored. Thanks to the Copilot review for catching it.
HaoZeke
left a comment
HaoZeke
left a comment
There was a problem hiding this comment.
Solid evaluator fixes:
+=extending list/tuple deps matches real easyconfig trees (silent drop of the first block was a real bug)- tuple
%with left-to-right conversion order, plus arity mismatch → skip the field (not guess) - regression tests cover the happy paths and the Copilot-caught arity cases
- style/docs/CI greps look like they were already needed on main
CI is green end-to-end. Merging.
Two evaluator fixes for constructs that show up in real trees:
dependencies += [...]replaced the previous list instead of extending it (only the Str+Str arm concatenated), so the first block was silently dropped. Lists and tuples now extend.'%s-%s' % (name, version)hit the unsupported-operands arm and failed the whole recipe. Tuple args now substitute positionally, and%s/%dreplacement picks the leftmost conversion so mixed formats keep argument order.Three regression tests added;
cargo test --locked --all-targetspasses locally.