Skip to content
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

Clean up minor inconsistencies in cql2 schemas. #794

Merged

Conversation

eseglem
Copy link
Contributor

@eseglem eseglem commented Feb 25, 2023

While writing a parser I have spent a lot of time switching back and forth between the schema formats leading me to notice a few inconsistencies between the different formats so I have attempted to address them. I tried not to do anything dramatic, just align everything together a little better.

The biggest issue I have had is % and div only existing in the cql2.bnf file, but there were lots of other little things that added up. I have attempted to provide detailed justifications for everything below.

Both cql2.json and cql2.yaml

cql2.json

  • Switched prefixItems to items when it was used for objects of the same type.
    • This better aligns with the yaml schema.
    • It felt easier to read.
  • Replaced all additionalItems with minItems and maxItems.
    • prefixItems does not require all items in the array to exist. So, minItems is needed to require all of them.
    • More consistent within the schema and better aligns with the yaml schema.
    • The 2020 json-schema draft removed additionalItems so it seemed better to remove and be forward compatible.
  • Aligned function array items with yaml. arrayLiteral -> arrayExpression.
    • The rest of the items are Expressions in both the json and yaml so I figured this would be the one to adjust.
    • Note: Switching to Literal (or Instance per Change names of some "literal" production because they are not stricly literals. #790) types and adding function and property directly would better align to the bnf file and feels easier to read. But I did not want to do that without discussion. This would also apply to various other locations where similar things are done as well.

cql2.yaml

  • Moved comments related to json prefixItems to description within the yaml.
    • Based on other examples I have seen this seems to be the more acceptable place for that information.
    • Note: A move to OpenAPI 3.1 would support prefixItems and no longer need these comments. I figured that would need additional discussion though.
  • Added patternExpression to isLikeOperands.
    • While it is already a subset of characterExpression this aligns with the json better, and makes the description more useful.
    • Would need to be included anyways for prefixItems if an OpenAPI 3.1 upgrade happened.
  • Add minItems and maxItems to inListOperands.
    • This aligns with the other schemas. I think it was just missing before.
  • Quoted all the arithmetic and binary operators for consistency within the schema.
  • Adjusted order of arithmetic and binary operator enums for consistency with json and bnf.

@cportele cportele added the CQL2 label Feb 27, 2023
@cportele
Copy link
Member

Meeting 2023-02-27: Thank you for the PR. The changes look correct. There was some issue with using items which is why it was changed to prefixItems, but we need to check what the issue was. @pvretano will review and validate the updated examples (see #783) and check, if everything is now consistent.

@cportele
Copy link
Member

Meeting 2023-04-10: @pvretano will review.

Copy link
Contributor

@pvretano pvretano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change from prefixItems to items is OK.

@cportele cportele merged commit cd87451 into opengeospatial:master May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants