chore(env): drop dead mergeJSONTopLevel + error on mixed JSON selectors#130
Merged
Conversation
filterJSONHybrid had a "mixed simple + complex" code path that called
mergeJSONTopLevel, but no test ever exercised it and no real-world use
case justified maintaining it. JSON has no comments to preserve, so
the historical reason for the dual-path was always thin: users either
want a clean dot-path extraction (gjson) or a full JMESPath query, not
both on the same JSON file.
Changes:
- Delete mergeJSONTopLevel entirely (~25 lines).
- filterJSONHybrid now errors out on mixed selectors with a clear
message: "use either dot-paths only or JMESPath only".
- New test TestFilterContent_JSON_MixedSelectorsErrors pins the
error contract.
Behavior:
- Simple-only JSON selectors: unchanged (filterJSON via gjson).
- Complex-only JSON selectors: unchanged (filterJSONJMESPath).
- Mixed JSON selectors: now errors instead of going through dead code.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes an untested “mixed selector” merge path for JSON selects and makes mixed simple+JMESPath selectors on JSON files fail fast with a clear error, while adding a regression test to lock the new behavior.
Changes:
- Deleted the dead
mergeJSONTopLevelhelper and the mixed-mode JSON merge branch. - Updated
filterJSONHybridto return an explicit error when simple and complex selectors are mixed for.json. - Added
TestFilterContent_JSON_MixedSelectorsErrorsto pin the new error contract.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/env/select.go | Removes JSON mixed-mode merge code and introduces an explicit error for mixed selector lists on JSON. |
| pkg/env/select_jmespath_test.go | Adds test coverage asserting mixed selectors on JSON return an error mentioning “mixing”. |
Address copilot review on #130: the filterContent doc comment advertised mixing simple+complex selectors as a general feature, but filterJSONHybrid now intentionally rejects mixed selectors for .json files. Update the comment so callers aren't misled. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous assertion only checked for the substring "mixing", which any unrelated JSON parse error would also satisfy. Pin the specific guidance string the error contract promises so a future refactor can't quietly change the wording. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…round 3) Update the public-facing docs to match filterJSONHybrid's actual behavior: JSON files reject mixed simple+complex selectors with an explicit error. The previous wording in env-sync.mdx implied mixed mode worked for any file format. Also drop a stale #135 follow-up reference from the source comment. Co-Authored-By: Claude Opus 4.6 (1M context) <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
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.
Follow-up from PR #127 review (round 7, comment about dead `filterJSONHybrid` mixed-mode merge).
What
`filterJSONHybrid`'s mixed-simple-and-complex code path called `mergeJSONTopLevel`, but no test ever exercised it and no real-world use case justified maintaining it. JSON has no comments to preserve, so the dual-path was always thin.
Changes
Behavior
This is one of the small follow-ups mentally tracked from the #127 review loop.
🤖 Generated with Claude Code