feat(discover): handle more npm/npx/pnpm/pnpx patterns#355
feat(discover): handle more npm/npx/pnpm/pnpx patterns#355aeppling merged 7 commits intortk-ai:developfrom
Conversation
|
LGTM — solid coverage of npm/npx/pnpm/pnpx patterns with thorough tests (622 pass, fmt + clippy clean). One thing needed before merge: rebase on master — there's a conflict (master has moved since v0.25.0). git fetch origin master && git rebase origin/masterOnce rebased, this is ready to merge. |
|
@pszymkowiak done! |
|
Could you add bun and bunx format as well ? |
|
@navidemad could that be done in another PR? I'm not giving up and will try to propose something but I don't know bun nor use it, I'll have to read the doc a little bit before having something ready |
|
@pszymkowiak rebased on develop, should be ready to merge |
|
Rebased on latest develop — all existing fixes preserved (redirect stripping #530, find-pipe protection #563, Ruby support #724) alongside your new npm/pnpm/pnpx patterns. 1115 tests passing (4 new from this PR). No conflicts, no regressions. 14/14 rewrite commands tested: all npm/npx/pnpm/pnpx patterns working correctly. Ready to merge. Thanks @KuSh! |
|
Hey We are cleaning up the codebase and improving the project structure for better onboarding. As part of this effort, PR #826 reorganizes No logic changes — only file moves and import path updates. What you need to doRebase your branch on git fetch origin && git rebase origin/developGit detects renames automatically. If you get import conflicts, update the paths: use crate::git; // now: use crate::cmds::git::git;
use crate::tracking; // now: use crate::core::tracking;
use crate::config; // now: use crate::core::config;
use crate::init; // now: use crate::hooks::init;
use crate::gain; // now: use crate::analytics::gain;Need help rebasing? Tag @aeppling |
|
@aeppling rebased! ready for review |
|
Hey @KuSh Bug found: npm test / npm t / npm tst → rtk vitestnpm test -- --coverage → rtk vitest -- --coverage Problem: npm test runs whatever is in package.json#scripts.test — could be jest, mocha, ava, tap, anything. The PR blindly rewrites it to rtk vitest. If the project uses jest, the hook will intercept npm test and run vitest instead, silently breaking the test suite. Reproduced: rtk vitest runs package_manager_exec("vitest") → executes vitest run --reporter=json. This is NOT what npm test does. A jest project would get vitest invoked instead. Proxy contract violation. |
This is a pre-existing bug as original rewrite already contains that: pattern: r"^(pnpm\s+|npx\s+)?(vitest|jest|test)(\s|$)",It's also the case for jest, they're mostly compatible but the user request to run jest and rtk runs vitest instead. I can see to fix that but it will require to discuss what we want to achieve before |
5364de2 to
9b1e1bb
Compare
- handle npm exec|run and their aliases - handle pnpm exec|run and their aliases like npm - handle pnpx and its alias like npx - handle all forms of js script/package execution Signed-off-by: Nicolas Le Cam <niko.lecam@gmail.com>
Signed-off-by: Nicolas Le Cam <niko.lecam@gmail.com>
Signed-off-by: Nicolas Le Cam <niko.lecam@gmail.com>
…npm test` commands as we don't know which test framework is used under the hood Signed-off-by: Nicolas Le Cam <niko.lecam@gmail.com>
…jest Also remove duration computation as there's no endTime attribute in json output Signed-off-by: Nicolas Le Cam <niko.lecam@gmail.com>
This reverts commit 94a3532. Build is no longer a pnpm command with specific handling. Signed-off-by: Nicolas Le Cam <niko.lecam@gmail.com>
|
All is ready for merge ! @KuSh Thanks for addressing reviews and already existing issues :) |
Also don't blindly rewrite npm test to vitest and distinguish between jest and vitest