Repository navigation
Conversation
|
Hey @guyoron1 thanks for addressing this more durable fix, will avoid further maintenance. npm run inject part -> validlint part -> lost rewriteRemoving lint from the rule fixes the substitution, but it deletes the rewrite entirely, not just the substitution. pnpm lint, npm lint, bare lint -> match no rule -> run raw The npm part shows the pattern to follow: there the rewrite and the substitution are separate, so dropping the injection kept the rewrite + filter. On lint they're fused, the rewrite target rtk lint is the substitution. proposed fixMirror your npm half: keep the rewrite, drop only the substitution. 1 - npm run lint / pnpm run lint: already fine after this PR,nothing to do. 2 - Bare pnpm lint: the only routing gap. Extend the generic pnpm rule to wrap bare invocations → rtk pnpm lint. Important: wrap any bare pnpm , not just lint, enumerating script names would recreate the exact kind of list this PR removes. 3 - Compression recovery (follow-up) enhance the npm/pnpm output filter to detect lint-style diagnostics (file:line:col + rule id) and compress around them: collapse clean/progress lines, keep diagnostics, tee tail hint the tail. Thanks for your work |
|
Heyaaaa :) thanks for the detailed feedback! lint routing gap — Fixed: the pnpm discover rule now matches Specific tool rules (eslint, biome, vitest, tsc, etc.) still take priority via RegexSet last-match semantics, so Added tests for both the generic routing and the specific-tool override behavior. Compression recovery (point 3) makes sense as a follow-up. |
603719c to
51e383f
Compare
51e383f to
fbff471
Compare
|
@aeppling rebased on current develop and reworked the pnpm half; the description is updated to match. Short version:
One thing I'd rather flag than have it surprise you: three of #3275's tests flip on purpose, because their premise ("bare Overlaps I noticed: #2776 (which you called a temporary fix; if it lands first this hard-conflicts in If these all land, the order that avoids conflicts is #2655, then this, then #3150 (the one shared spot is the |
`pnpm lint` and `npm run lint` were rewritten to `rtk lint`, discarding the package.json script's own flags, chains, and exit code. Direct linter invocations (`eslint`, `biome`) continue to route through rtk lint; only the bare script-name variants are dropped from the rule. Fixes rtk-ai#2094 Split out of a larger commit that also removed NPM_SUBCOMMANDS from npm_cmd.rs (fixes rtk-ai#2663) — that half is dropped here since rtk-ai#2663 was independently closed by 56519c4 (extending the allowlist instead of removing it), and unconditionally deleting the allowlist would also regress the "assume script name" convenience 56519c4's approach kept.
… rewrite Dropping the script names from the lint rule leaves `pnpm lint` matching no rule at all, so it loses its rewrite entirely instead of just losing the substitution. Broaden the pnpm rule from a fixed subcommand list to any bare `pnpm <word>`, so every package.json script routes through `rtk pnpm` and runs verbatim — real flags, real chain, real exit code. Enumerating script names would only trade one list for another. The first token must not start with `-`: a global flag is not a subcommand, and `rtk pnpm --filter @app` or `rtk pnpm -x install` would die at clap. Flag-first forms still reach the rule through strip_pnpm_global_opts, whose fixed set is unchanged (rtk-ai#3275). Specific tool rules (eslint, biome, vitest, tsc, ...) keep priority via RegexSet last-match semantics.
fbff471 to
8a38ae4
Compare
guyoron1
changed the title
2 participants
Fixes #2094
lint
Removes
lint(the script name) from the lint rewrite rule, sopnpm lint/npm run lintare no longer rebuilt as an eslint invocation: the package.json script's own flags, chain and exit code are preserved. Direct linter calls (eslint,npx eslint, ...) still route throughrtk lint.Per aeppling's review, dropping
lintmust not drop the rewrite altogether, so the generic pnpm rule now wraps any barepnpm <word>asrtk pnpm <word>, with no enumerated script names. It runs the real script through the passthrough and keeps its exit code. The pattern is^pnpm\s+[^-\s]\S*(a bare word, never a flag), which keeps the flag-first guards from #3275 green (pnpm --filter @appandpnpm -x installstill don't rewrite).pnpm lintrtk lint(script flags lost)rtk pnpm lint(real script, exit code kept)pnpm -r lintrtk pnpm -r linteslint src/,pnpm eslint .rtk lint ...rtk lint ...(unchanged)pnpm --filter @app,pnpm -x installThree of #3275's tests flip on purpose, each with a comment saying why: bare
pnpm buildandpnpm -r lintnow route tortk pnpm, which is the "wrap any barepnpm <word>" from the review.Tests
rtk pnpm lintrunspnpm lintand keeps the script's exit codecargo fmt --all -- --check,cargo clippy --all-targets(0 warnings),cargo test --all(4036 passed, 8 ignored)Scope note (2026-09-27)
This originally also deleted
NPM_SUBCOMMANDSinnpm_cmd.rs(fixes #2663). That half is dropped: #2663 was independently closed by 56519c4, which extended the allowlist instead of removing it. Rebasing the deletion on top of that commit would revert 56519c4's approach rather than merge with it, so I split it out rather than force the conflict.For what it's worth: 56519c4's extended list still doesn't cover the alias forms (
npm add,npm x), so those still getrun-injected on current develop. If the "remove the allowlist entirely" direction (matching aeppling's stated preference on #2664) is still wanted for #2663, I can open that as its own PR — just didn't want to silently re-litigate a closed issue and overwrite a merged commit's approach inside this one.Not done here:
rtk discovernow credits the pnpm rule's headline savings to bare scripts that run through the passthrough (0% savings). Making that accurate needs a per-subcommand table, which is another list. The follow-up aeppling described (compressing lint-style diagnostics in the npm/pnpm filter) is what would make the number real.