Skip to content

feat(checkSyntax): create checkSyntaxStdin - #318

Open
nurazon59 wants to merge 1 commit into
extrabacon:masterfrom
nurazon59:nurazon59/feat/check-syntax-stdin
Open

feat(checkSyntax): create checkSyntaxStdin#318
nurazon59 wants to merge 1 commit into
extrabacon:masterfrom
nurazon59:nurazon59/feat/check-syntax-stdin

Conversation

@nurazon59

@nurazon59 nurazon59 commented Dec 9, 2025

Copy link
Copy Markdown
Contributor

resolve #295

@nurazon59
nurazon59 marked this pull request as draft December 9, 2025 13:35
@nurazon59
nurazon59 marked this pull request as ready for review December 15, 2025 04:36

@StantonMatt StantonMatt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this against #295 locally.

Validation passed:

  • npm run compileOnce
  • npx mocha -r ts-node/register test/test-python-shell.ts --grep "checkSyntaxStdin"
  • npm test
  • npx prettier --check index.ts test/test-python-shell.ts
  • git diff --check origin/master...HEAD

No upstream checks are reported on this PR branch.

The behavior looks useful: the stdin path avoids creating pythonShellSyntaxCheck*.py temp files and the tests cover valid/invalid single-line and multiline syntax.

One thing I would tighten before merge: the test named should use pythonOptions from defaultOptions does not currently prove or implement that behavior. checkSyntaxStdin() builds the spawned argv as ["-c", "import sys; compile(...)"] and does not include defaultOptions.pythonOptions. I confirmed that with PythonShell.defaultOptions = { pythonOptions: ["--definitely-not-a-python-option"] }; checkSyntaxStdin("x=1") still passes because the option is ignored.

If option support is intended for this helper, it should include defaultOptions.pythonOptions in the spawned argv and test with a sentinel option that would fail if ignored. If not, I would rename or remove that test so the new API expectation stays clear.

@kikiminyes

Copy link
Copy Markdown

I found one mismatch between the added test and the implementation.

should use pythonOptions from defaultOptions sets PythonShell.defaultOptions = { pythonOptions: ['-B'] }, but checkSyntaxStdin() builds the subprocess args as only:

['-c', 'import sys; compile(sys.stdin.read(), "<stdin>", "exec")']

So the test currently only proves the method still succeeds while default options are set; it does not prove those options are passed to Python. I verified this with a mocked child_process.spawn: the recorded args are ['-c', ...], with no -B.

npm run compileOnce passes. I could not run the Python-backed tests in this local Windows environment because Python spawning is still failing here, so this check is limited to TypeScript compile plus the mocked spawn assertion.

I think either the implementation should include toArray(this.defaultOptions.pythonOptions) before -c, or the pythonOptions test should be removed/renamed if this API intentionally ignores default Python options.

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

Labels

None yet

3 participants