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

Cleaning up samples, parity between yaml and json - Close #559 #389

Merged
merged 2 commits into from
Feb 13, 2016
Merged

Cleaning up samples, parity between yaml and json - Close #559 #389

merged 2 commits into from
Feb 13, 2016

Conversation

jharmn
Copy link
Contributor

@jharmn jharmn commented Jun 5, 2015

Hopefully addresses issues identified in #267
All samples run through swagger-tools validate with no errors (unless I missed one ;))

yaml2json and json2yaml used to convert between formats to ensure parity.

@webron
Copy link
Member

webron commented Jun 5, 2015

You mean the npms or their online variants like http://yamltojson.com/?

@mohsen1
Copy link
Contributor

mohsen1 commented Jun 5, 2015

The conversation from yaml to json should be part of build and we should never touch the json manually

@jharmn
Copy link
Contributor Author

jharmn commented Jun 5, 2015

@webron the npms.
@mohsen1 the only reason I did it the other way, is there were samples existing in both dirs that weren't in the other.

@jharmn
Copy link
Contributor Author

jharmn commented Jun 5, 2015

@mohsen1 but I agree that would be a great enhancement.

@webron
Copy link
Member

webron commented Jun 5, 2015

Well, I definitely don't have time now to add it to the build process, unfortunately.

In my experience, the online versions produce subpar conversions, where http://i-tools.org/unserialize produced much better results. Not sure if/what cli tool they have. In any case, I'll have to go over the PR and check it, but will get around to it only next week.

@jharmn
Copy link
Contributor Author

jharmn commented Jun 5, 2015

@webron There are solid NPM packages for this (I haven't had any issues yet, forgive my ignorance if there are issues):
https://www.npmjs.com/package/yaml2json
https://www.npmjs.com/package/json2yaml

I'll see if I can get some time to add it to the build next week.

@webron
Copy link
Member

webron commented Jun 5, 2015

@jasonh-n-austin - yes, but if those npms are the same one used in the online variant, then solid or not, they produced imperfect conversions (specifically from json to yaml). Since yaml is a superset of json, you can basically have "json" values to properties (like arrays and such), and to me, as samples, that doesn't teach users what the yaml should look like. Does it work? yes. Is it what we want to teach? Probably not. Keep in mind though that I haven't looked at your conversions yet, I'm just speaking from experience of converting all the JSON in the spec itself to YAML.

And please don't get me wrong, I'm so happy you took the time to submit this PR, it is a huge help.

@jharmn
Copy link
Contributor Author

jharmn commented Jun 5, 2015

@webron no offense taken...definitely let me know if you see something weird, I'd love to know if I'm using sub-standard stuff 😰

@jharmn
Copy link
Contributor Author

jharmn commented Jul 22, 2015

@webron ping!

@webron
Copy link
Member

webron commented Jul 22, 2015

@jasonh-n-austin - sorry, fairly swamped right now. I'm going to allocate some time specifically for the spec issues, especially to fixes to the schema, samples, and tests, but due to my upcoming trip it won't be for at least a week. Some great PRs are pending.

If this is more urgent to you, please let me know.

@jharmn
Copy link
Contributor Author

jharmn commented Feb 5, 2016

Rebased and ran gulp yaml2json to ensure JSON is generated. Reviewing to see how valid this still is, looks like there's been updates to samples here and there.

@@ -0,0 +1,58 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This existed in YAML, but not JSON examples.

@jharmn
Copy link
Contributor Author

jharmn commented Feb 5, 2016

@webron I was thinking we should revisit having accurate examples, as we look to add functionality. Updating these with new features will be pretty important, and it would suck if they weren't working in the first place.
P.S. It seems like we need a README in examples on how to change (edit YAML, run gupl yaml2json) for contributors.

@Maks3w Maks3w mentioned this pull request Feb 11, 2016
@jharmn jharmn changed the title Cleaning up samples, parity between yaml and json Cleaning up samples, parity between yaml and json - Close #559 Feb 11, 2016
@webron
Copy link
Member

webron commented Feb 13, 2016

@jasonh-n-austin -yeah, time to merge it, following #559 as well.

As for the examples in general, it should be part of the next spec development process. If we can automate conversions and so on, all the better (so we can agree the source for the samples is YAML for example).

webron added a commit that referenced this pull request Feb 13, 2016
Cleaning up samples, parity between yaml and json - Close #559
@webron webron merged commit 47a268c into OAI:master Feb 13, 2016
@ePaul ePaul mentioned this pull request Apr 19, 2016
AndersDJohnson pushed a commit to AndersDJohnson/OpenAPI-Specification that referenced this pull request Apr 8, 2019
Cleaning up samples, parity between yaml and json - Close OAI#559
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.

3 participants