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

Refactoring replaceRef for definitions and circular references #74

Closed
wants to merge 8 commits into from

Conversation

javagl
Copy link
Contributor

@javagl javagl commented May 2, 2022

This is a DRAFT for a somewhat broader refactoring of the replaceRef function. (It is based on the state from #71, but that could easily be separated).

The current replaceRef function has some issues:

The changes in this PR are supposed to address these issues. The implementation now tries to keep track of the replacements that have been done, mainly by storing the references in a "schemaRepository", and to properly substitute definitions even when they are across multiple files.

I added some basic test cases for the new functionality. But further tests will have be be done to verify that other changes in the output, compared to the "golden" state that is checked in, are not just "changes", but actually improvements.

The prominent role of schema.title for identifying a schema makes it difficult to track the state that the output is generated from. The role of the (undocumented) schema.typeName is something that I'll still have to wrap my head around. (And although I'm a bit curious to see how things would collapse when an input schema contained a property called typeName, that's probably something that may not be considered to be important in the near future...)

@javagl
Copy link
Contributor Author

javagl commented May 3, 2022

If someone could explain the difference between mergeProperties and mergeProperties, (intuitively), that would be great. Besides: I wonder whether "schema3..." is actually supposed to be supported/maintained medium- or long term.

@emackey
Copy link
Contributor

emackey commented May 3, 2022

I don't think most folks are still using schema3 (certainly glTF is not). I don't know why schema3's mergeProperties is different, but perhaps schema4 had some work done or bugfixing or something that never got backported to schema3. Not sure.

Although this is a draft, I'd like to point out some important information loss apparent in the tests. For example:

main nested-keyword.md

|**extensions**|`extension`|Dictionary object with extension-specific objects.|No|
|**extras**|`extras`|Application-specific data.|No|

This branch nested-keyword.md

|**extensions**|`any`||No|
|**extras**|`any`||No|

One easy way to spot diffs is to check in your source code changes to git, then issue npm run test, then copy all files from test/test-output to test/test-golden, then ask git to show you what's changed in your working tree. The diff tool you've configured for git should show any changes that got copied into the golden folder, and if those changes are not intentional, then there are things to fix in the source.

@javagl
Copy link
Contributor Author

javagl commented May 31, 2022

This is obsolete due to a larger refactoring at https://github.com/javagl/wetzel/tree/generate-3dtiles

The output of this branch for glTF is actually pretty close to the original output, but it involves too many code changes to immediately become a PR here.

@javagl javagl closed this May 31, 2022
@JC3
Copy link

JC3 commented Jun 30, 2022

Does this have anything to do with #81 ? It appears that either $ref isn't working correctly at all, or $id is ignored.

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