Refactor handling of additionalProperties and add support for propertyNames

[theory]

The new getAdditionalProperties() function, derived from behavior that was in createPropertiesDetails(), is now called at the top level to consistently show additional properties for the top-level schema as well as sub-properties. The behavior has been tweaked in a couple ways, however:

• The Additional properties are (not )? allowed statements have been moved into bullet items, to match other property constraints. They also now always appear for every item.
• The value for the Type of each property item is now the type specified by the additionalProperties object, rather than its parent. This seems clearly more correct to me, but it changed the output of tests for extensions, so that instead of listing the type as "Extension" it now says "object". There might be something funky going on with how composition of schemas affects things, but it's not clear to me what's going on here. So I left a comment showing the previous behavior, in case we need to go back to it.
• The value of the Type of each property item can now be linked. It was supposed to be before but did not appear to work properly.

The handling of most of the basics of describing a schema have been moved into a new function, getBasicSchemaInfo(), so that it can be called recursively for propertyNames.

A new schema is added to the tests, referenced by an additionalProperties element in test/test-schemas/v2020-12/image.schema.json to demonstrate these changes. image.schema.json also includes a propertyNames element to ensure it works properly (even if its constraints don't make much sense). propertyNames was added in Draft 2019-09, not 2020-12, but this seemed like the simplest place to put it.

I suggest disabling whitespace changes in the diff view for a clearer view of the actual changes.

[theory]

This is the bit I don't understand. The type of additional properties of an extension is clearly an object, but it was previously rendering here as an Extension.

wetzel/test/test-schemas/nested/extension.schema.json

Lines 8 to 10 in 189f3f8

	"additionalProperties": {
	"type": "object"
	}

[javagl]

tl;dr: I think that the new output is indeed more correct and makes more sense.

A quick side-by side of the old and the new state (from the markdown version) :

---

Looking at the code that is commented out after this change :

if ((additionalProperties.type === 'object') && defined(schema.title)) {
    formattedType = style.linkType(schema.title, schema.title, autoLink);
}

This handles the case that there is no specific type information about the additionalProperties (i.e. it is only object). This could be the case for something like

...
"additionalProperties": {
    "type": "object",
    "title": "Additional property"
}

It might be that the intention originally was to have code like this:

if ((additionalProperties.type === 'object') && defined(additionalProperties.title)) {
    formattedType = style.linkType(additionalProperties.title, additionalProperties.title, autoLink);
}

to insert the title of the additiona property type there (instead of the title of the containing schema)

[theory]

Yeah that makes a lot more sense. I think I might've tried it and got a bunch of unexpected failures, but would have to revisit to know for sure.

[theory]

Tried your variant. When parsing [image.schema.json]'s metadata property, the properties of which are meta.schema.json, it emits:

### Image.metadata

* **Type**: `object`
* **Required**: No
* **Additional properties are allowed.**
* **Type of each property**: `meta`

Which seems odd to say the least, because the meta.schema.json schema does in fact have a title, "title": "Metadatum", but it's just emitting the schema name, meta`, instead. I guess that's valid as the type, but shouldn't it use the title?

[theory]

Demonstration that the additional property type properly creates a link now (though I tried and failed to get it to use the title instead of the type).

[theory]

This is the new feature I wanted to support. I am designing objects in which the values are all the same types but the keys are a specific list. The propertyNames.enum pattern perfectly suits this use case. Just need to get it in generated docs.

[javagl]

I tried something similar in #72 , but this is only a draft.

This PR here is more mature.

As mentioned in the inlined comment: I think that the change in the "golden" output is correct.

Things that I like about this one:

• Cleanups. smaller functions with names. Additional tests.

Things that I dont' like:

• It adds two different things (both are fine, but reviewing them independently could be easier).
• The tests are mixing things....

The latter is a general issue: It's not always clear what they are testing. Nobody would expect an image.schema.json to specifically test support for propertyNames. I think it would be better to have a propertyNames.schema.json that tests this particular functionality (and that can, when necessary, be run and debugged isolatedly).

It may not be necessary to change this here, but I think that at some point the tests as they are currently done in the main branch should be split, in order to dedicatedly have smaller tests for the features. This would be a bit closer to "real unit-tests", while still keeping the "backward compatibility guarantees" that are established with the golden output (and that make perfectly sense, IMHO...)

[theory]

It adds two different things (both are fine, but reviewing them independently could be easier).

Yeah, I started to add support for additionalProperties and quickly realized it needed some refactoring to work well. Could probably split them up with a bit of effort, though don't know how much it's worth it if they will both be merged.

The tests are mixing things....

Oh is it the name of the test file that's the issue?

[javagl]

Could probably split them up with a bit of effort, though don't know how much it's worth it if they will both be merged.

I don't think that this is necessary. Part of this comment may have been due to the fact that I'm just getting started with wetzel, and am not always sure which part of the code does what, and this makes it harder for me to compare, say, the changes here to my approach, or identify which parts of the changes here are 1. pure cleanups, 2. support for additionalProperties, or 3. support for propertyNames.

Oh is it the name of the test file that's the issue?

As I said, it's a somewhat general issue. The test files are obviously largely extracted from (or related to) glTF, probably with the goal to ~"have a few test cases that cover the features that are used for glTF". But the files have been changed, renamed, and extended with ... unrelated things, which makes it hard to change or add features.

For example, image.schema.json is largely the glTF "image", but it contains this fractions stuff - I don't know what this is about, but I think this could/should be tested dedicatedly in a small, standalone fractions.schema.json where the proper handling of fractions can be tested and verified dedicatedly and easily.

For the PR that I linked to, I created the following schema files (not part of the PR yet), for testing additionalProperties:

exampleAdditionalPropertiesSimple.schema.json:

{
  "$schema": "https://json-schema.org/draft/2020-12/schema",
  "title": "ExampleAdditionalPropertiesSimple",
  "description": "An example with additionalProperties being a simple schema instead of a boolean value",
  "type": "object",
  "properties": {
    "exampleProperty": {
      "type": "string"
    }
  },
  "additionalProperties": {
    "type": "integer"
  }
}

exampleAdditionalPropertiesComplex.schema.json:

{
  "$schema": "https://json-schema.org/draft/2020-12/schema",
  "title": "ExampleAdditionalPropertiesComplex",
  "description": "An example with additionalProperties being a complex schema instead of a boolean value",
  "type": "object",
  "properties": {
    "exampleProperty": {
      "type": "string"
    }
  },
  "additionalProperties": {
    "type": "object",
    "properties": {
      "exampleInteger": { "type": "number" },
      "exampleString": { "type": "string" },
      "exampleEnum": { "enum": ["ExampleEnumValueA", "ExampleEnumValueB"] }
    }
  }
}

There should be a ...Disallowed and ...Trivial versions of these files, with additionalProperties:false and true, respectively. But the general idea is to have small, isolated tests for the specific functionality. They can be "documented" (somehow) via the description, more closely resembling the idea of a unit test, and allow working on (and testing) changes for a specific functionality.

Again: This is nothing that can sensibly be covered in this PR. Carving out more such "unit tests" from the current main state should be a separate change, because (and this is the important part) this should only be changes in the test schemas, without any code changes, to make sure that this change does not cause regressions, but instead, only allows to identify regressions more easily in the future.

(Like... you know, when a type description changes from Extension to object - even though that's not a regression, but rather a bugfix...)

EDIT: Another example of such a "trivial test schema" for circular references: https://github.com/CesiumGS/wetzel/pull/74/files#diff-8c71fe596e97f3342e62b7b6ff201e7963450558d46dbbd7a6b2bdc465e016fa - this is also not yet part of a non-draft PR, but might be in the near future.

[javagl]

I think that summary is already declared here

[javagl]

(More generally, there are several errors (e.g. missing semicolons) that could be caught with eslint)

[theory]

I don't think that this is necessary. Part of this comment may have been due to the fact that I'm just getting started with wetzel, and am not always sure which part of the code does what, and this makes it harder for me to compare, say, the changes here to my approach, or identify which parts of the changes here are 1. pure cleanups, 2. support for additionalProperties, or 3. support for propertyNames.

That's fair.

As I said, it's a somewhat general issue. The test files are obviously largely extracted from (or related to) glTF, probably with the goal to ~"have a few test cases that cover the features that are used for glTF". But the files have been changed, renamed, and extended with ... unrelated things, which makes it hard to change or add features.

I think it's more important to have proper coverage in unit tests than that the test cases be semantically meaningful.

But the general idea is to have small, isolated tests for the specific functionality. They can be "documented" (somehow) via the description, more closely resembling the idea of a unit test, and allow working on (and testing) changes for a specific functionality.

I could definitely see that being useful! Sounds like it might require quite a bit of re-thinking of how to structure this test cases, though. Is that something you're willing to do, and that @emackey and others who maintain this project would welcome? Might make sense to propose it in an issue rather than this PR.

[javagl]

I think it's more important to have proper coverage in unit tests than that the test cases be semantically meaningful.

I think that both the coverage and the semantic meaning are important. Pushing it to the extreme: A single everything.schema.json wouldn't be ideal...

Sounds like it might require quite a bit of re-thinking of how to structure this test cases, though. Is that something you're willing to do, and that @emackey and others who maintain this project would welcome? Might make sense to propose it in an issue rather than this PR.

I think the general structure of the test cases is fine - I mean the basic approach of having the 'golden' output and the infrastructure for generating and comparing this output. I was mainly referring to the granularity of the Schema files for the tests. Breaking this into smaller, semantically meaningful pieces should not be sooo difficult. I tried to summarize a few thoughts (and points that we discussed here) in #80