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

Fix wrong setAllOf() call, javadoc and add enums values to javadoc #2

Merged
merged 1 commit into from
Jun 15, 2017

Conversation

Arcnor
Copy link
Contributor

@Arcnor Arcnor commented Jun 14, 2017

No description provided.

@@ -40,7 +40,7 @@
* <pre><code>
* appendToFragment("uri", "newFragment") = "uri#/newFragment"
* appendToFragment("uri#xyz", "newFragment") = "uri#xyz/newFragment"
* <code></pre>
* </code></pre>
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 fixes Maven Javadoc generation (at least on J8)

@@ -435,7 +435,7 @@ private void processBasicSchema(URI uri, Schema schema)
List<Schema> subSchemas =
SchemaGeneratorUtils.getSubSchemas(
uri, node, "oneOf", schemaResolver);
schema.setAllOf(subSchemas);
schema.setOneOf(subSchemas);
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 two were breaking the whole generation

if (nonObjectEnumStrings != null && !nonObjectEnumStrings.isEmpty()) {
descriptionLines.add("Valid values: " +
nonObjectEnumStrings);
}
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 chunk adds the "Valid values" bit when those are defined as part of anyOf (they were already checked on the verification step, but not added to the docs, which might be confusing)

@javagl
Copy link
Owner

javagl commented Jun 14, 2017

Thanks for this contribution!

I think I already fixed the core issue (setOneOf) locally, but didn't push yet, because I admittedly didn't expect someone to really use this library. I hope you noticed the disclaimer: This library contains some very "experimental" and "pragmatic" parts that are not perfectly documented and/or hand-tweaked to obtain the output just in the way that I need it for JglTF...

(By the way: Updating the JSON Schema of glTF to Version 6 was alrady considered - so there might be a v6 folder soon...)

In any case, I'll probably merge this PR soon. Re-running the generator on the latest schema was on my TODO list anyhow, in view of KhronosGroup/glTF#605 (comment)

If you have any further feedback or comments, I'd be happy to hear them. Particularly, I wonder whether you actually used this library in a different context than glTF...?

Thanks again!

@javagl javagl merged commit e518b19 into javagl:master Jun 15, 2017
@Arcnor
Copy link
Contributor Author

Arcnor commented Jun 15, 2017

I used this to rerun the generator on the JGLTF bindings, so no, nothing new. I also added a few things to parse the gltf_detailedDescription fields, but those changes don't work if you try to keep this schema generator generic, so I didn't include them.

@javagl
Copy link
Owner

javagl commented Jun 15, 2017

Yes, I also considered to somehow extract the gltf_detailedDescription. It adds really valuable information. But although the generator is likely only used for glTF, you're right: It should not become toooo specific for glTF, and I usually try to find some sort of abstraction for the specific parts (and I still have to find a proper workaround for 7892497 ... )

I've opened an issue for that, under #3 (but it currently does not have highest priority - the work on JglTF itself and many other tasks are more important right now)

@Arcnor
Copy link
Contributor Author

Arcnor commented Jun 15, 2017

Well, the "right" way of doing this (at least that commit you've linked) is having a separate "semantic JSON" file, where you specify actions to make for certain paths that your generator can read and act on. For the gltf_detailedDescription and other fields outside the JSON Schema specification however, there is no nice solution, although what I did is not that hacky: I've added a new "extras" field to the schema node, which contains all the unknown properties on a Map.

Using that, with that semantic file I'm talking about it should be "trivial" (well, wit some work of course :D) to map to any part of the generator.

Anyway, just my 2c.

@javagl
Copy link
Owner

javagl commented Jun 15, 2017

The "extras" Map sounds like what I mentioned as one option in the issue #3 . But still, at which point (and how) to you convert this into a JavaDoc? At some point, you have to know that you have to look up gltf_detailedDescription in this map.

I'm not sure what you mean by the "semantic JSON" file (i.e. what exactly it should contain, and how to interpret it). In general, there are certainly several (more or less sophisticated) possible ways for "steering" the code generation. More mature libraries like https://github.com/joelittlejohn/jsonschema2pojo already offer many options here.

I hope that some of these options can be added with reasonable effort, based on the interfaces that I introduced, but admittedly, these interfaces are not completely thought out: The primary goal was to have a code generator, for personal use, only for glTF - any generalization was just caused by internal brainstorming ;-) Beyond that, I really wanted the JavaDoc and validation statements to be in there (although I'm not sure whether jsonschema2pojo does not support something like this as well, with the appropriate configuration...)

Other "design goals", like separating the "Schema generation" and the "Code generation" may make things harder than they had to be, but I thought that a strict separation here could add some flexibility in the long run. (E.g. the update from v3 to v4 was not so much effort, although, admittedly, I didn't fina a reasonable way to achieve the goal of supporting both versions simulataneously - at least, not without lots of code duplication...)

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