Implement yaml support for schema and target doc [READY FOR REVIEW]

[zendril]

To address issue #4

This is a rudimentary implementation that will:

• Look for .yaml or .yml in the schema, if found, convert to json and use it
• look for .yaml or .yml in the target doc, if found, convert to json and use it

Other notes:

• This needs heavy refactoring, but it does work.
• I have do revert out your change that pulls in the neilpa.me/go-x/fileuri dependency because I cannot resolve it yet. Will re-implement/merge once you make that public.
• Removed many/most fileuri references as it was breaking my usage on windows.
  -- Will re-implement once I understand better how they are supposed to work and how doc references are supposed to be resolved. This is just my naive mis-undestanding of how it is supposed to work.
• I can't use the gox in makefile either, but I plan on making a different PR for a more comprehensive makefile that works across OSs and can be used in github actions for buiding/publishing.

[zendril]

Oh, and error handling/flow is terrible. Needs to be unified :)

[neilpa]

Thanks @zendril for putting together this change. I know it's still a WIP but I added a few comments. I suspect they may overlap with what you had in mind when you mentioned "heavy refactoring".

Overall though, if we get those things cleaned up and figure out the fileuri issue I'll be happy to merge this.

[neilpa]

I have do revert out your change that pulls in the neilpa.me/go-x/fileuri dependency because I cannot resolve it yet. Will re-implement/merge once you make that public.

@zendril can you give this another try? I did a quick test by making a new OS user account that doesn't have my github creds setup and was able download the dependency. I'm also wondering if you hit the time frame between when I added the dependency here but before setting up the redirect and/or making the neilpa/go-x repo public. And then potentially cached a bad 403 response.

[zendril]

Oh, and tests need to be updated to include yaml.

[zendril]

@zendril can you give this another try? I did a quick test by making a new user account that doesn't have my github creds setup and was able download the dependency. I'm also wondering if you hit the time frame between when I added the dependency here but before setting up the redirect and/or making the neilpa/go-x repo public. And then potentially cached a bad 403 response.

Will do.

For my info, is there a reason this is be extracted out?

[neilpa]

For my info, is there a reason this is be extracted out?

I've copy/pasted bits of code like this into a few different projects lately. Wanted to experiment with using a "grab bag" package to avoid the duplication. If it turns into more trouble than it's worth though I'll move the code directly in here and/or explicitly vendor the dependencies. I may do the latter anyway since I prefer having everything that's needed to build the project as self-contained as possible.

[neilpa]

Will also need to update the printUsage message and README to make note of the YAML support

[neilpa]

Removed many/most fileuri references as it was breaking my usage on windows.

I just realized this could be removed completely in favor of the raw []byte loader approach that you've added for the YAML change. That would be simpler than trying to deal with the file:// URIs and the cross-platform issues that arise. e.g.

func jsonLoader(path string) (gojsonschema.JSONLoader, err) {
	buf, err := ioutil.ReadFile(path)
	if err != nil {
		return nil, err
	}
	switch filepath.Ext(path) {
	case ".yml", ".yaml":
		buf, err = yaml.YAMLToJSON(buf)
	}
	if err != nil {
		return nil, err
	}
	return gojsonschema.NewBytesLoader(buf), nil
}

[zendril]

I updated to go 1.13 on my end as well.. so between whatever you did and my changes it works.. PR should be updated with your fileuri code back in.

[neilpa]

And of course I added that loader alternative comment right after you brought back the fileuri dependency :/. Feel free to do as you wish with that suggestion.

[zendril]

Yeah.. I'd rather remove it based on your comment..
With fileuri, I found that it was working to load the json file, but would give me an error when loading the yaml file. So I say we can unify on removing it and using byte loader. Good?

[zendril]

@neilpa Done with all the basic refactoring. I updated the printed help and readme and I think I addressed all of the other comments. I'm going to take a break for an hour or so. Take a fresh look and I'll get to any new comments after that. Thanks..

[neilpa]

A few more minor comments/suggestions. Thanks a ton for tackling this! Especially adding more tests 😄

[zendril]

@neilpa Thanks a bunch for being patient and giving some great code feedback and collaboration.

I didn't add tests to main_test.go because I'm on my work/windows machine and haven't switched over to my home linux machine. If you can add them and run the tests that would be great. If not then I'll try to do so later.

I also do have experience with any of the $ref stuff, and don't have examples of that but I think it would be good to have a test for that as well because I have no way of know if I busted any of that.

I'd be happy to create the tests and mock data, but might need some help and/or a few hours to read up more on how these should be constructed (especially given that these will be local and not pulled from the $id).

[neilpa]

Of course, and thanks for putting this together.

If you're interested in writing up more tests that's great and I'd be happy to add them. I've mostly been banking on the existing tests in underlying gojsonschema package. I added the initial ones just to make sure that the most basic of functionality was working.

[zendril]

You know what.. for the test vs windows test.. we don't need separate ones now.. with the pathing changes a / works just fine on windows.. updating the tests and will commit and see if it makes our CI overlords happy.

[neilpa]

updating the tests and will commit and see if it makes our CI overlords happy.

😆

[zendril]

Works for everything except where the globs are in play because that is returning
Tinkering around but I think they also need to be run through abs, unless you have a better idea.

[neilpa]

but I think they also need to be run through abs,

That sounds reasonable to me.

[zendril]

@neilpa Pushed.. altho can you take a look.. I may have made the test flakey and/or not know how to make it run the test on both platforms..

Sometimes I get a space in between output and sometimes not..

testdata/data-fail.yml: fail: (root): foo is required
testdata/data-error.yml: error: load doc yaml: found unexpected end of stream

and sometimes:

testdata/data-fail.yml: fail: (root): foo is required

testdata/data-error.yml: error: load doc yaml: found unexpected end of stream

Who knows.. probably because of this // +build windows !windows

[neilpa]

Who knows.. probably because of this // +build windows !windows

Yea, I'm not sure how that is going to interact when go processes the build tags. That should probably be removed altogether.

I'm actually going to merge this as is though. This PR is getting a little big and I'd like to tackle additional changes to the tests as a follow-up. I've got an idea for further unifying them that I'd like to see if I can make work. Hopefully it will drastically simplify the boiler plate that exists in the tests.