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

Replaced get_element with JSONPointere.jl package #25 #52

Closed
wants to merge 11 commits into from

Conversation

YongHee-Kim
Copy link
Contributor

Hello! It's been four years since I've proposed #25

It caught my attention recently and I've updated JSONPointer.jl and decided to tackle this issue as well

This pull request does not applied JuliaFormatter, but my next pull request for resolving #37 did used formatter accordingly

@odow
Copy link
Collaborator

odow commented Jun 27, 2024

I understand your motivation for this. But I'm still not really sure that I want to add a third-party dependency to save 37 lines of code.

What are the benefits of using JSONPointer over the current implementation?

@YongHee-Kim
Copy link
Contributor Author

YongHee-Kim commented Jun 29, 2024

I have given some thought and you are right there isn't much benefits it brings to the JSONSchma.jl by adding JSONPointer dependency at the moment

Only potential benefits I could think of is providing better information when a 'reference' is broken for example

using JSON 
using JSONSchema

data = """
{
  "fruits": [ "apple", "orange", "pear" ],
  "vegetables": [
    {
      "veggieName": "potato",
      "veggieLike": true
    },
    {
      "veggieName": "broccoli",
      "veggieLike": false
    }
  ],
  "ref": { "\$ref": "#/fruits/0"},
  "ref2": { "\$ref": "#/fruits/3"},
  "ref3": { "\$ref": "#/vegetables/0/veggieName"},
  "ref3": { "\$ref": "#/vegetables/1/veggieNam"}
}
""" 
JSONSchema.Schema(data)

Will fail because #/fruits/3 and #/vegetables/1/veggieNam is wrong

Current error messages are:
image
image

With a JSONPointer commit, it will be
image
image

Albeit, I understand your reluctance for adding a dependency. I'm okay if you decide not to merge this pull request 😸

@odow odow closed this in #53 Jun 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants