Fix runString temp path to use tmpdir() and add regression test#320
Fix runString temp path to use tmpdir() and add regression test#320dusan-maintains wants to merge 1 commit into
Conversation
- Fix runString() using tmpdir reference instead of tmpdir() call (fixes extrabacon#320) - Replace exec() with execFile() to prevent command injection in checkSyntaxFile, getVersion, and getVersionSync - Add temp file cleanup in runString() and checkSyntax() via .finally() - Replace custom extend() with Object.assign - Re-enable getVersion/getVersionSync tests (were disabled since extrabacon#158) - Add GitHub Actions CI matrix (Node 18/20/22, Python 3.10/3.12, 3 OSes) - Update minimum Node.js engine from 0.10 to 16 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix runString() using tmpdir reference instead of tmpdir() call (fixes extrabacon#320) - Replace exec() with execFile() to prevent command injection in checkSyntaxFile, getVersion, and getVersionSync - Add temp file cleanup in runString() and checkSyntax() via .finally() - Replace custom extend() with Object.assign - Re-enable getVersion/getVersionSync tests (were disabled since extrabacon#158) - Add GitHub Actions CI matrix (Node 18/20/22, Python 3.10/3.12, 3 OSes) - Update minimum Node.js engine from 0.10 to 16
StantonMatt
left a comment
StantonMatt
left a comment
There was a problem hiding this comment.
I checked this locally and compared it with the later duplicate refresh in #330.
This branch fixes the actual bug: tmpdir is imported as a function, so the old tmpdir + sep + ... path construction stringifies the function instead of using the OS temp directory. The new tmpdir() call makes runString() write to the expected temp path, and the test avoids spawning Python while still checking the generated file and handoff to PythonShell.run().
Local checks on this branch:
npm run compileOnce
npx mocha -r ts-node/register test/test-python-shell.ts --grep "temporary file"
npm test
npx prettier --check index.ts test/test-python-shell.ts
git diff --check origin/master...HEAD
All passed locally; full npm test reported 44 passing. GitHub reports no check runs for this PR, so there is no upstream CI result to claim.
The overlap with #330 is straightforward: both branches make the same minimal behavior fix and both pass the full local suite here. #330 uses path.join(tmpdir(), ...) and checks option identity in the regression test, while this PR keeps the existing sep style and checks equivalent handoff behavior. From a maintainer perspective, either branch can carry the fix; if this older PR is preferred, the only small thing I would consider is borrowing the path.join(tmpdir(), ...) shape from #330 for path construction consistency.
2 participants
Summary
PythonShell.runString()temp file path generation by callingtmpdir()instead of using the function reference.PythonShell.runwithout spawning Python.Why
filePathwithtmpdir + sep + ..., which uses the function object instead of the temp directory path.writeFileSync, makingrunString()fragile.Test Plan
npm test -- --grep "temporary file in the OS temp directory"1 passing.Compatibility