Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add OUTERMOST_OBJ and OUTERMOST_ARR allowances #9

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

alanpog
Copy link

@alanpog alanpog commented Sep 16, 2024

Not sure if relevant for the general library here, but sending this PR in case you find it useful (it was needed for our specific use case). These changes intend to allow partials for the outermost objects and arrays (i.e., objects that don't have another object up in the json hierarchy and similarly for arrays), but disallow it for any non-outermost objects or arrays (avoiding the words root and non-nested as their definitions are usually slightly different).
The README.md has some explanation and there are quite a few test cases too.
I couldn't find a .prettierrc in the repo and ended up unintentionally messing up with the formatting :(

Summary by Sourcery

Add new allowances OUTERMOST_OBJ and OUTERMOST_ARR to support partial parsing of outermost JSON objects and arrays. Enhance the JSON parsing logic to track depth and improve handling of partial structures. Update documentation to reflect these changes.

New Features:

  • Introduce OUTERMOST_OBJ and OUTERMOST_ARR allowances to enable partial parsing of the outermost JSON objects and arrays, providing more granular control over JSON parsing.

Enhancements:

  • Improve JSON parsing logic to track object and array depth, allowing for more precise handling of partial JSON structures.

Documentation:

  • Update README.md to include explanations and usage examples for the new OUTERMOST_OBJ and OUTERMOST_ARR allowances, clarifying their purpose and application.

Copy link

stackblitz bot commented Sep 16, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

vercel bot commented Sep 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
partial-json-coverage ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 17, 2024 1:00pm

Copy link

sourcery-ai bot commented Sep 16, 2024

Reviewer's Guide by Sourcery

This pull request adds new allowances for parsing outermost objects and arrays in partial JSON, while maintaining stricter parsing for nested elements. It introduces OUTERMOST_OBJ and OUTERMOST_ARR flags, updates the parsing logic to handle these new allowances, and improves error handling and partial parsing capabilities.

File-Level Changes

Change Details Files
Added new allowances for outermost objects and arrays
  • Introduced OUTERMOST_OBJ and OUTERMOST_ARR flags in options.ts
  • Updated Allow object to include new flags
  • Added explanations for new allowances in README.md
src/options.ts
README.md
Updated parsing logic to handle new allowances
  • Implemented depth tracking for objects and arrays
  • Modified parseObj and parseArr functions to check for outermost elements
  • Updated error handling to consider new allowances
src/index.ts
Improved error handling and partial parsing capabilities
  • Enhanced parseStr function to handle more partial string scenarios
  • Updated parseNum function for better partial number parsing
  • Improved overall error messages and partial JSON handling
src/index.ts
Code refactoring and formatting improvements
  • Restructured parseAny function for better readability
  • Updated variable names and comments for clarity
  • Applied consistent code formatting throughout the files
src/index.ts
src/options.ts

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @alanpog - I've reviewed your changes and they look great!

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: 2 issues found

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

src/options.ts Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
@CNSeniorious000 CNSeniorious000 self-assigned this Sep 17, 2024
@CNSeniorious000 CNSeniorious000 added the enhancement New feature or request label Sep 17, 2024
@CNSeniorious000
Copy link
Member

CNSeniorious000 commented Sep 17, 2024

Thanks @alanpog! I think this is useful at least myself. I once had use cases where I needed to disallow incomplete child containers(object/array) while allowing the top-level container to be partial. At that time I resolved array-related problems by simply dropping the last item or something else, and resolved object-related ones by using zod to validate them.

Your proposal is great and this implementation works well. But due to my perfectionist tendencies, I often find myself wondering if there's an even better way to solve these kinds of problems. For example:

  1. If we support allowing the outermost container to be incomplete. What if we want to allow the two outer layers to also be incomplete?
  2. What If the user wants a value of a specific key to be allowed to be incomplete but not other keys?

It seems quite impossible to achieve this through a single allow flag.

So I think this implementation is still not perfect enough.


By the way, the diff is too large to review. That's my fault not to include a prettier config and I've added it just now.

@CNSeniorious000
Copy link
Member

CNSeniorious000 commented Sep 17, 2024

I have some ideas about supporting these complex allow strategies elegantly, but I'm not sure:

Approach One — Input Predicate Function

The parse will take an options object to extend configuration besides simple allow. We may support inputing a validate predicate function to choose whether to allow an object / array. The signature of options may look like this:

interface options {
  allow: number;
  validate(parsed: any, text: string, parents: Parent[]): boolean
}

type Parent = {
  type: "OBJ";
  key: string;
} | {
  type: "ARR";
  index: number;
}

For example, if user do this:

parse(`[0, {"a": [{`, { allow: ALL, (parsed, text, parents) => { ... } })

The validate function will be called at most 4 times with

validate({}, '{', [{ type: "ARR", index: 0}, { type: "OBJ", key: "a" }, { type: "ARR", index: 1}])
validate([{}], '[{', [{ type: "OBJ", key: "a" }, { type: "ARR", index: 1}])
validate({"a": {}}, '{"a": [{', [{ type: "ARR", index: 1}])
validate([0, {"a": {}}], '[0, {"a": [{', [])

Approach Two — Return PartialInfo

Inject some information into the return value. Like this:

export const partial = Symbol('__partial__');

And the partial information may be like this:

interface PartialInfo {
  text: string;
}

Then users can filter the result themselves using this information.

For example, if using this way:

> res = parse(`[0, {"a": [{`, { allow: ALL, inject: true }) // [0, {"a": [{}]}]
> res[0][partial] // undefined
> res[1][partial] // { text: '{"a": [{' }
> res[1].a[partial] // { text: '[{' }
> res[1].a[0][partial] // { text: '{' }

They can drop at any depth level as they wish.

@CNSeniorious000 CNSeniorious000 changed the title Add OUTERMOST_OBJ and OUTERMOST_ARR allowances. Add OUTERMOST_OBJ and OUTERMOST_ARR allowances Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants