Skip to content

fix(npm): correct operator precedence in progress indicator filter - #2655

Open
guyoron1 wants to merge 1 commit into
rtk-ai:developfrom
guyoron1:fix/npm-operator-precedence
Open

guyoron1 wants to merge 1 commit into
rtk-ai:developfrom
guyoron1:fix/npm-operator-precedence

Conversation

@guyoron1

@guyoron1 guyoron1 commented Jun 27, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Closes #2654

filter_npm_output dropped any line containing ⸨ or ⸩ regardless of its length, because && binds tighter than ||:

line.contains("⸩") || line.contains("⸨") || line.contains("...") && line.len() < 10

so the len() < 10 guard only applied to the ... arm. The three contains calls are now hoisted into a named is_progress_glyph, and the length guard applies to the whole condition. (The parenthesised one-liner is 102 characters and rustfmt won't take it; the comment spells out the precedence so it isn't tidied back.)

Does this cost compression? No: npm only draws the ⸨…⸩ bar when stderr is a TTY, and rtk always captures through a pipe. Checked with npm 10.8.2: a piped npm install emits no ⸨ / ⸩ and no CR, so that arm is unreachable under rtk either way. This makes the code say what it meant.

Tests

  • test_progress_indicator_preserves_long_lines fails on develop's condition (Long line with progress chars should be preserved, got: ok)
  • cargo fmt --all -- --check, cargo clippy --all-targets (0 warnings), cargo test --all (all pass)

@guyoron1
guyoron1 force-pushed the fix/npm-operator-precedence branch from 930875d to 766e013 Compare August 14, 2026 19:03
`&&` binds tighter than `||`, so the length guard only applied to
lines matching `"..."` while `⸨`/`⸩` matches were unconditionally
dropped.  Wrapping the disjunction in parentheses applies the
`line.len() < 10` check to all three progress-indicator patterns.

Closes rtk-ai#2654
@guyoron1

Copy link
Copy Markdown
Contributor Author

Rebased on current develop; the bug is still there verbatim at npm_cmd.rs:190. One change from what #2654 proposes: the parenthesised one-liner comes out at 102 characters and rustfmt won't take it, so the three contains calls are hoisted into a named let and the length guard stays on the if. Same semantics, and the comment now spells out the precedence so it doesn't get tidied back.

On whether this costs compression: it can't. npm only draws the ⸨…⸩ bar when stderr is a TTY, and rtk always captures through a pipe. Checked with npm 10.8.2: a piped npm install emits no ⸨ / ⸩ and no CR at all, so that arm is unreachable under rtk either way and this just makes the code say what it meant.

This touches npm_cmd.rs like #2860, but only its test module overlaps: whichever merges second needs a trivial rebase (keep test_progress_indicator_preserves_long_lines).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(npm): operator precedence bug in progress indicator filter

1 participant