-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments aren't crucial, so fix whichever you think are worth the time.
util/action.schema.json
Outdated
"$id": "https://kulturkrock.se/action.schema.json", | ||
"$schema": "http://json-schema.org/draft-07/schema#", | ||
"title": "Action in Opus", | ||
"description": "An action object in an Opus to use for Screencrash", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't this description says much. How about something like "An action such as starting or stopping an effect"? Same in the other places like this.
util/action.schema.json
Outdated
{ | ||
"$id": "https://kulturkrock.se/action.schema.json", | ||
"$schema": "http://json-schema.org/draft-07/schema#", | ||
"title": "Action in Opus", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to mention that it's in an opus (and in Screencrash, below this)? When it's used in the opus schema I think it's clear from the context.
util/commands/audio.schema.json
Outdated
"oneOf": [ | ||
{ | ||
"title": "Sync asset", | ||
"description": "Sync asset to audio component resource folder. Don't do this manually.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"manually" is a bit ambiguous, "in an opus" instead?
util/commands/audio.schema.json
Outdated
"title": "Sync asset", | ||
"description": "Sync asset to audio component resource folder. Don't do this manually.", | ||
"properties": { | ||
"target": { "enum": ["audio"] }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These enums will always have only one value, right? Then I think it's clearer to use the "const" keyword instead. Same in the other places like this.
util/opus.schema.json
Outdated
"nodes": { | ||
"type": "object", | ||
"patternProperties": { | ||
"^[0-9]+.*$": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, IDs don't need to start with numbers if the node has defined pdfPage and pdfLocationOnPage. I'm sure this is possible to encode with JSON schema somehow, or it could just be in the description if that is too complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving this for the future,
util/opus.schema.json
Outdated
"action_templates": { | ||
"type": "object", | ||
"patternProperties": { | ||
"^.*$": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this cover everything? Use additionalProperties instead of patternProperties? E.g.
"additionalProperties": {
"$ref": "action.schema.json#"
}
} | ||
}, | ||
"additionalProperties": false, | ||
"required": ["next", "prompt"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we handle it properly, but we should allow for a node to not have "next". There has to be a last node, otherwise the audience can't go home.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving this for the future,
Existing commands for all components should be included. Each component is put in its own source file to enable easy extendability. The current opus passes the checks as valid.