fix(spec): schema field description omits load() - #80
Open
ficosta wants to merge 1 commit into
Open
Conversation
The `schema` field in the Graphic Manifest is described as defining the data parameters of the 'update' method. That description omits load(), which takes a `data` argument following the same schema, and it names a method that does not exist, presumably meaning updateAction(). graphics-manifest.ts is a plain regeneration. The server-api.ts line was applied by hand, because a full regeneration against the current toolchain also produces unrelated changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
schemafield in the Graphic Manifest is described as defining "the dataparameters of the 'update' method". The specification describes the same field
differently in
Specification.mdline 74, as "the JSON schema definition for thedataargument to theload()andupdateAction()methods".The substantive problem is the omission of
load(). This schema governs thedataargument passed to
load()as well as the one passed toupdateAction().Specification.mdlines 263 to 265 state it directly: thedatagiven toload()"follows the model described in the Manifest using the
schemafield". As written,the description suggests the schema only applies to the update path, which could lead
an implementation to validate data on update but not on load, or a controller to
offer schema-driven input only for updates.
The method name is the smaller issue, since
'update'was presumably meant asupdateAction(). Still, no method by that name exists: the Graphic methods areload(),dispose(),playAction(),stopAction(),updateAction(),customAction(), and, for non-real-time Graphics,goToTime()andsetActionsSchedule(), and the Server API graphicInstance operations areclear,load,updateAction,playAction,stopActionandcustomActions/{customActionId}.This is not only an internal inconsistency.
server-api.yamlreferencesgraphics/schema.jsonby$ref, so the string is generated into both committeddefinition files and ships in the published package. In
ograf@1.2.0it appears atdist/generated/graphics-manifest.d.ts:401anddist/generated/server-api.d.ts:1568,which means anyone implementing OGraf sees it as an editor tooltip on the
schemafield, pointing at a method that does not exist.
The line dates back to the initial import of the documentation and appears to predate
the current method naming.
What changed
The description now reads "the data parameters of the 'load()' and 'updateAction()'
methods", matching
Specification.mdand following the convention already used inthis file, where
supportsNonRealTimerefers to "the 'goToTime()' and the'setActionsSchedule()' methods".
Three lines, one per file:
v1/specification/json-schemas/graphics/schema.json(the source)v1/typescript-definitions/src/generated/graphics-manifest.ts(regenerated)v1/typescript-definitions/src/generated/server-api.ts(see note below)npm run buildpasses.Note on the generated files
graphics-manifest.tsis the output ofnpm run generate-types, unmodified.For
server-api.tsI applied the description line by hand rather than committing theoutput of
npm run generate-server-types. Running that regenerator against thecurrent toolchain produces unrelated changes:
action-2is collapsed intoaction,the duplicate
action-2schema block is dropped, and an@exampleblock appears onone description. That suggests the committed file was generated with a different
openapi-typescriptversion than a freshnpm installresolves today, since thedependency is a caret range. Rolling that drift into a one line documentation fix
seemed wrong, but it may be worth a separate look.