-
Notifications
You must be signed in to change notification settings - Fork 27
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pvretano fixed/rebased.
core/openapi/schemas/themes.yaml
Outdated
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I'm now following what is in master, which means themes.yaml
.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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). |
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.
Looks like a good improvement
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?