fix up / make consistent themes definitions #246
Conversation
core/examples/json/queryables.json
Outdated
| "queryable": "themes", | ||
| "type": "datetime" | ||
| "queryable": "keywords", | ||
| "type": "string" |
There was a problem hiding this comment.
How has the type been determined? Isn't that actually an array of strings? Maybe I'm missing something though...
There was a problem hiding this comment.
I've updated the type based on the type of the actual queryable. For example, if keywords is an array, the queryable is of type array. Users searching would pass a string literal (keywords=birds). @pvretano is this the right interpretation of defining queryables?
There was a problem hiding this comment.
That makes sense, I've just seen other implementations (in STAC?), where the queryables are 1:1 mappings to the actual "property" (here array). This left me confused. If this PR is the correct way forward, great.
|
I guess a reference would be okay and simpilfy the example. Or is there any good reason why in practice someone wouldn't use a ref and reuse existing schemas? |
|
@m-mohr I've updated the example to match the queryables schema as well as the various types. As well:
For an implementation, yes we would ref and reuse existing schemas. The question is whether to use a |
|
01-MAY-2023: Closes #235 |
|
My review is above (#246 (comment)) and did not receive a comment yet. Otherwise looks good. |
core/openapi/schemas/catalogue.yaml
Outdated
There was a problem hiding this comment.
@tomkralidis the schema for the themes member should be:
themes:
type: array
description: A knowledge organization system used to classify the catalogue.
items:
$ref: theme.yaml
The PR I recently merged created a new file named theme.yaml in the master branch.
There was a problem hiding this comment.
@pvretano fixed/rebased.
core/openapi/schemas/themes.yaml
Outdated
There was a problem hiding this comment.
@tomkralidis remove this file and verify that the theme.yaml file that is now in the master branch contains the same information for a single theme.
There was a problem hiding this comment.
FYI I'm now following what is in master, which means themes.yaml.
There was a problem hiding this comment.
Wait, I see the following now in master:
...but there is no themes.yaml, only theme.yaml.
Summary: I've updated this PR to point to theme.yaml. Should be all aligned now.
There was a problem hiding this comment.
@tomkralidis looks good. Do we still need the themes.yaml file? Go ahead and merge once @m-mohr does his review.
|
@pvretano good catch, removed. Thanks P.S. if/once this PR is merged, it should be "Squash and Merge"d (lots of superfluous commits). |
Follow-on of #237, as per #235 (comment)
I also refactored the themes definition into its own schema fragment/file for reuse. In theory we could use this in https://github.com/opengeospatial/ogcapi-records/blob/master/core/standard/clause_12_local_resources_catalogue.adoc#example, but the example would then show a schema reference rather than the actual inline definition (which is valuable from a spec readability perspective, but another place to update if things change i the themes definition).
Thoughts @pvretano @m-mohr?