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

Make JSONLoader public to support cusotm json loader #103

Merged

Conversation

yuichi1004
Copy link

I worked on making JSON Loader to be public so that we can implement custom JSON loader.
The topic was already discussed on #99 but not implemented yet.

The changes are little bit bigger than I expected for the following reason.

  • I needed to implement JSONLoaderFactory so that Schema uses custom json loader to resolve their ref
  • I needed to move schema construction logic to schema.go from jsonLoader.go. Because Schema construction process requires several private func/struct access (like newSchemaPool(), newSchemaReferencePool())

I think this approach is reasonable enough but any opinion?

@xeipuuv xeipuuv merged commit 8029392 into xeipuuv:master Jun 20, 2016
@petemoore
Copy link

petemoore commented Jun 20, 2016

I think this PR has bust the string loading.

Simple test case with an empty schema:

package main

import (
    "log"

    "github.com/xeipuuv/gojsonschema"
)

func main() {
    _, err := gojsonschema.NewSchema(gojsonschema.NewStringLoader(`{ "$schema": "http://json-schema.org/draft-04/schema#" }`))
    if err != nil {
        log.Fatal(err)
    }
}

Note, when rolling back this PR, the above code sample executes successfully, as expected. With the PR landed, it fails with:

2016/06/20 20:57:48 Reference {0xc820082900 {[]} false false false false false} must be canonical

Therefore, I think this PR needs to be rolled back. Ideally it would be good to add this as a unit test too. Thanks!

@yuichi1004
Copy link
Author

I'm working on bug fixing on the above test case.
It's OK to rolling back the PR meanwhile.

Thanks!

@vbatts
Copy link

vbatts commented Jun 21, 2016

for cross reference sake: opencontainers/image-spec#162 (comment)

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.

4 participants