fix(utils): narrow bare except Exception in merge_json_files - #4189
Open
Noor-ul-ain001 wants to merge 1 commit into
Open
fix(utils): narrow bare except Exception in merge_json_files#4189Noor-ul-ain001 wants to merge 1 commit into
Noor-ul-ain001 wants to merge 1 commit into
Conversation
merge_json_files's read of the existing JSON file caught bare `Exception` around `json5.load`, so a real bug there (e.g. a `TypeError`/`AttributeError`) was silently treated the same as a normal parse failure -- `None` returned, existing settings preserved untouched, nothing surfaced unless `verbose`. Only `OSError` (inaccessible file) and `ValueError` (malformed JSON5 -- json5's decode error is a `ValueError` subclass) are expected outcomes here; anything else should propagate. Same bug, same fix shape, as the caller `handle_vscode_settings`, whose own bare `except Exception` was just narrowed to `(OSError, ValueError, KeyError)` in commit 16f4577 (PR github#3844) with the same rationale ("let programming errors like TypeError or AttributeError propagate"). That PR's own regression test monkeypatched `merge_json_files` to prove the caller's narrowing works; this fixes and tests the callee itself, which still had the original bare-except bug. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
merge_json_files's read of the existing JSON file wrapsjson5.loadin a bareexcept Exception as e:that returnsNone(meaning "preserve existing settings, skip merge") on any failure — including a real programming bug like aTypeError/AttributeError, which is silently treated identically to an expected I/O or parse error and, withoutverbose, surfaces nothing at all.The only expected outcomes for this block are:
OSError— the file is inaccessible (already special-cased forFileNotFoundErrorjust above)ValueError— malformed JSON5 (json5's decode error is aValueErrorsubclass)Anything else should propagate instead of being swallowed as if it were a parse failure.
This is the same bug, same fix shape, as its caller
handle_vscode_settings, whose own bareexcept Exceptionwas narrowed to(OSError, ValueError, KeyError)in commit 16f4577 (#3844) with the identical rationale — "let programming errors like TypeError or AttributeError propagate while still handling expected I/O and parse errors gracefully." That PR's own regression test monkeypatchedmerge_json_filesitself to prove the caller's narrowing works, but the callee it monkeypatched around still had the original bare-except bug.Test plan
test_merge_json_files_propagates_programming_errorstotests/test_merge.py, monkeypatchingjson5.loadto raiseTypeErrorand asserting it propagates out ofmerge_json_filesinstead of returningNoneDID NOT RAISE <class 'TypeError'>) and passes with itpytest tests/test_merge.py— 13 passedruff checkon both changed files — clean🤖 Generated with Claude Code