FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Propagate JSON parse errors by StantonMatt · Pull Request #326 · extrabacon/python-shell · GitHub

  1. extrabacon
  2. python-shell

Repository navigation

Propagate JSON parse errors - #326

Open
StantonMatt wants to merge 1 commit into
extrabacon:masterfrom
StantonMatt:fix-json-parse-error-propagation
ConversationCommits (1)ChecksFiles changed
Open

Propagate JSON parse errors#326
StantonMatt wants to merge 1 commit into
extrabacon:masterfrom
StantonMatt:fix-json-parse-error-propagation

Conversation

StantonMatt commented Jun 1, 2026

Copy link
Copy Markdown

Summary

  • catch exceptions from stdout parser functions before emitting message events
  • emit a parseError event and pass the first parse failure to .end() / PythonShell.run() so callers can handle invalid JSON output
  • keep user message listener exceptions outside the parser catch path so normal EventEmitter behavior is preserved

Fixes #253.

Validation

  • npm test -- --grep "parseError|JSON output"
  • npm test
  • git diff --check
  • review-fix-loop clean after addressing one reviewer finding

StantonMatt mentioned this pull request Jun 2, 2026
Open

kikiminyes commented Jun 2, 2026

Copy link
Copy Markdown

I checked the core path locally.

  • npm run compileOnce passes.
  • I could not run the Python-dependent suite in this Windows environment because Python spawning fails before the tests run, so I validated the changed JS path with a mocked child process instead.
  • With invalid JSON on stdout, parseError is emitted, .end() receives PythonShellParseError, and PythonShell.run(..., { mode: "json" }) rejects with the same parse error and empty logs.

The implementation looks directionally correct to me. Keeping parse failures on a separate parseError event avoids Node's special unhandled error event behavior while still surfacing the failure through the completion path.

PNHD suggested changes Sep 3, 2026
View reviewed changes

PNHD 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The parser/emit ordering on this head is sound, and the focused/full npm suites plus repository Prettier check pass locally on an LF-safe checkout. One required piece is still missing before this is ready as a library API change: the PR exports PythonShellParseError and adds a public parseError event, but README's event documentation does not mention either. For direct PythonShell callers without an .end() callback, parseError is the parser-failure surface; leaving it undocumented can turn the old crash into a failure users do not know how to observe. Please document the new event/class, when it fires, and how .end() / PythonShell.run() receive the first parse failure. I have an overlapping #334 with a different API shape and will defer to maintainer preference rather than requiring either architecture here.

PNHD mentioned this pull request Sep 3, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
PNHD PNHD requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Can't catch JSON.parse() error?

3 participants

Back | FazBrowse Home | New Git URL