[feat] Add support for parsing JSON with text prefix and postfix #15
Conversation
|
|
Reviewer's Guide by SourceryThis pull request introduces the ability to parse JSON strings that are embedded within other text. It adds two new options, Sequence diagram for JSON parsing with PREFIX and POSTFIXsequenceDiagram
participant Client
participant Parser
participant FixFast
Client->>Parser: loads('Text: {"key": "value"} End', PREFIX | POSTFIX)
Parser->>FixFast: fix_fast(text, allow)
Note over FixFast: Check for PREFIX
FixFast->>FixFast: Find first { or [
Note over FixFast: Check for POSTFIX
FixFast->>FixFast: Find last } or ]
Note over FixFast: Enable STR flag
FixFast->>FixFast: Process trimmed JSON
FixFast-->>Parser: Return fixed JSON
Parser-->>Client: Return parsed result
Class diagram showing Allow enum changesclassDiagram
class Allow {
<<enumeration>>
STR
NUM
NULL
BOOL
ARR
OBJ
NAN
INFINITY
_INFINITY
PREFIX*
POSTFIX*
INF
SPECIAL
ATOM
COLLECTION
ALL
}
note for Allow "* New flags added
ALL now includes PREFIX and POSTFIX"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ArmykOliva - I've reviewed your changes - here's some feedback:
Overall Comments:
- The fix_fast_old() function appears to be left in the code and should be removed
- Consider adding documentation about limitations of the PREFIX/POSTFIX parsing approach when dealing with nested JSON structures or strings containing braces
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Review instructions: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| first_brace = json_string.find('{') | ||
| first_bracket = json_string.find('[') | ||
|
|
||
| if first_brace != -1 and (first_bracket == -1 or first_brace < first_bracket): | ||
| json_string = json_string[first_brace:] |
There was a problem hiding this comment.
issue: Consider handling escaped braces/brackets in string literals when searching for JSON start/end
The current implementation using find() doesn't account for escaped characters in string literals. This could lead to incorrect parsing if the input contains escaped braces or brackets. Consider using a string-aware parsing approach similar to the old version.
| @@ -20,6 +20,78 @@ def join_closing_tokens(stack: List[Tuple[int, str]]): | |||
|
|
|||
| def fix_fast(json_string: str, allow_partial: Union[Allow, int] = ALL): | |||
| allow = Allow(allow_partial) | |||
|
|
|||
| # Handle PREFIX by finding first { or [ | |||
| if PREFIX in allow: | |||
There was a problem hiding this comment.
suggestion (performance): Consider combining PREFIX and POSTFIX string operations to reduce string slicing
When both PREFIX and POSTFIX are enabled, we perform multiple string slicing operations. Consider calculating both bounds first and then performing a single slice operation.
Suggested implementation:
allow = Allow(allow_partial)
# Calculate bounds for both PREFIX and POSTFIX at once
start_idx = 0
end_idx = len(json_string)
if PREFIX in allow:
first_brace = json_string.find('{')
first_bracket = json_string.find('[')
if first_brace != -1 and (first_bracket == -1 or first_brace < first_bracket):
start_idx = first_brace
elif first_bracket != -1:
start_idx = first_bracket
if POSTFIX in allow:
last_brace = json_string.rfind('}')
last_bracket = json_string.rfind(']')The code will need one additional change after the shown section:
- Add a single slice operation at the end:
json_string = json_string[start_idx:end_idx] - The existing POSTFIX handling code will need to be modified to set end_idx instead of directly slicing the string
| print(result) # Outputs: {'key': 'value'} | ||
| ``` | ||
|
|
||
| Note that `PREFIX` looks for the first `{` or `[` character and `POSTFIX` looks for the last `}` or `]` character to determine the JSON boundaries. |
There was a problem hiding this comment.
suggestion: Clarify the behavior when the starting/ending characters are not found.
It would be helpful to explicitly state what happens if the { or [ character is not found when using PREFIX, or if the } or ] character is not found when using POSTFIX. Does it raise an error? Or treat the whole string as JSON?
| Note that `PREFIX` looks for the first `{` or `[` character and `POSTFIX` looks for the last `}` or `]` character to determine the JSON boundaries. | |
| Note that `PREFIX` looks for the first `{` or `[` character and `POSTFIX` looks for the last `}` or `]` character to determine the JSON boundaries. If these characters are not found: | |
| - With `PREFIX`: A `JSONDecodeError` is raised if no starting `{` or `[` is found | |
| - With `POSTFIX`: A `JSONDecodeError` is raised if no ending `}` or `]` is found | |
| Examples: | |
| ```python | |
| # This will raise JSONDecodeError since there's no starting { or [ | |
| loads('This has no JSON', PREFIX) | |
| # This will raise JSONDecodeError since there's no ending } or ] | |
| loads('{"incomplete": "json"', POSTFIX) |
| @@ -20,6 +20,78 @@ def join_closing_tokens(stack: List[Tuple[int, str]]): | |||
|
|
|||
| def fix_fast(json_string: str, allow_partial: Union[Allow, int] = ALL): | |||
There was a problem hiding this comment.
issue (complexity): Consider extracting the prefix/postfix handling logic into helper functions to improve readability and maintainability.
The code can be simplified by extracting the token handling logic into focused helper functions and consolidating flag management. Here's a suggested refactoring:
def _find_prefix_start(json_string: str) -> int:
first_brace = json_string.find('{')
first_bracket = json_string.find('[')
if first_brace != -1 and (first_bracket == -1 or first_brace < first_bracket):
return first_brace
return first_bracket if first_bracket != -1 else 0
def _find_postfix_end(json_string: str) -> int:
last_brace = json_string.rfind('}')
last_bracket = json_string.rfind(']')
if last_brace != -1 and (last_bracket == -1 or last_brace > last_bracket):
return last_brace + 1
return last_bracket + 1 if last_bracket != -1 else len(json_string)
def fix_fast(json_string: str, allow_partial: Union[Allow, int] = ALL):
allow = Allow(allow_partial)
# Enable STR when handling PREFIX/POSTFIX
if PREFIX in allow or POSTFIX in allow:
allow |= STR
# Apply PREFIX/POSTFIX handling
start = _find_prefix_start(json_string) if PREFIX in allow else 0
end = _find_postfix_end(json_string) if POSTFIX in allow else len(json_string)
return _fix(json_string[start:end], allow, True)This refactoring:
- Extracts prefix/postfix logic into focused helper functions
- Consolidates flag handling at the start
- Simplifies the main function flow
- Eliminates the need for fix_fast_old
WalkthroughThe pull request enhances the Changes
Sequence DiagramsequenceDiagram
participant User
participant Parser
participant JSONExtractor
User->>Parser: loads(text, allow=[PREFIX, POSTFIX])
Parser->>JSONExtractor: Locate first '{' or '['
Parser->>JSONExtractor: Locate last '}' or ']'
JSONExtractor-->>Parser: Extracted JSON string
Parser-->>User: Parsed JSON object
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/partial_json_parser/core/myelin.py (3)
44-47: Align approach with fix_fast_old for consistency.
Currently,fix_fastalways setsSTRwhenPREFIXorPOSTFIXis used, whilefix_fast_oldremoves these flags after processing. Consider consolidating both approaches or adding clarifying comments about their intended differences.
55-64: Reduce duplicated logic for prefix handling.
fix_fast_oldrepeats the prefix search code used infix_fast. Extracting shared logic into a helper function could simplify maintenance and minimize duplication.
65-94: Unify string-escape checks to avoid potential edge cases.
The code at line 79 togglesin_stringbased on a single backslash check. Meanwhile,is_escaped()provides a more robust mechanism for evaluating escaped quotes. Consider consolidating these checks to ensure all valid escape sequences are properly handled.- if char == '"' and (i == 0 or json_string[i-1] != '\\'): - in_string = not in_string + if char == '"' and not is_escaped(i): + in_string = not in_string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(2 hunks)src/partial_json_parser/core/myelin.py(1 hunks)src/partial_json_parser/core/options.py(3 hunks)
🔇 Additional comments (6)
src/partial_json_parser/core/myelin.py (2)
24-33: Consider explicit handling when braces/brackets are absent.
If the input string contains no '{' or '[', the current logic proceeds without any trimming, which might lead to unexpected or partial results elsewhere.
34-43: Double-check behavior for missing closing tokens.
Similar to the prefix logic, if there is no '}' or ']', the function makes no modifications. Verify whether a fallback or error handling is needed for strings that never properly close a JSON scope.src/partial_json_parser/core/options.py (2)
16-17: New flags for PREFIX and POSTFIX look suitable.
Adding these members to the enum and incorporating them intoALLextends the parser's ability to handle text around JSON precisely as intended.Also applies to: 23-23
40-41: Exporting PREFIX and POSTFIX in the module's namespace.
This makes them readily accessible and keeps them consistent with other enum flags. No issues here.Also applies to: 60-61
README.md (2)
105-126: Clear examples of PREFIX and POSTFIX usage.
Including step-by-step code snippets clarifies how to parse JSON embedded within text. This effectively demonstrates the newly added functionality.
174-176: Extended documentation for new flags.
ListingPREFIXandPOSTFIXin theAllowenum reference makes it easy for users to discover the new capabilities.
|
Thank you! I am celebrating the Chinese new year these days, and will take a look when I have time~ |
|
Unit tests should also be updated to support this change. But IMO it's a very useful feature. At least for me since llms always like to answer with: {your actual json}""" which is annoying |
|
The postfix doens't work and breaks the actual program. upon further evaluation. I think postfix pruning is kind of impossible during the llm generation. Prefix is easily fixed though. |
Add PREFIX and POSTFIX options for handling text around JSON
This PR adds support for parsing JSON that is embedded within other text by introducing two new options:
PREFIX: Allows text before the JSON string starts (e.g.This is your JSON: {"key": "value"})POSTFIX: Allows text after the JSON string ends (e.g.{"key": "value"} - end of JSON)Implementation Details
{or[character to identify JSON start}or]character to identify JSON endExample Usage
Summary by Sourcery
Add support for parsing JSON with text prefix and postfix using the PREFIX and POSTFIX options.
New Features:
loadsfunction. The PREFIX option handles text before the JSON, while POSTFIX handles text after the JSON.Tests:
Summary by CodeRabbit
Release Notes
New Features
PREFIXandPOSTFIXoptions to handle JSON extraction from complex stringsDocumentation
Improvements