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

update CQL2 definition to reference OGC API yaml file in github, update npm build #261

Merged
merged 2 commits into from
Mar 3, 2022

Conversation

philvarner
Copy link
Collaborator

Related Issue(s):

Proposed Changes:

  1. Update CQL2 spec to reference version in ogcapi-features github repo
  2. npm build improvements

PR Checklist:

  • This PR is made against the dev branch (all proposed changes except releases should be against dev, not master).
  • This PR has no breaking changes.
  • This PR does not make any changes to the core spec in the stac-spec directory (these are included as a subtree and should be updated directly in radiantearth/stac-spec)
  • I have added my changes to the CHANGELOG or a CHANGELOG entry is not required.

@@ -35,7 +35,6 @@ build/Release
# Dependency directories
node_modules/
jspm_packages/
package-lock.json
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

recommended practice is to commit package-lock.json

Copy link
Collaborator

Choose a reason for hiding this comment

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

For software yes, but for a specification? There I'd assume you'd want the latest version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think for any build where you have dependencies. They're pinned to specific versions in it. Otherwise, anytime new build dependency versions could be resolved that no longer work with the current code, and there's no easy way to fix that, since you don't know what previous versions actually worked.

@@ -0,0 +1 @@
16
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

use nvmrc, and upgrade to node 16

Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reasoning? And why in a CQL PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was updating the openapi yaml, so I took the opportunity to update the build which mostly does validation while I was at it. I probably should have separated them into different PRs, but didn't.

@@ -12,14 +12,15 @@ info:
name: Apache License 2.0
url: 'http://www.apache.org/licenses/LICENSE-2.0'
tags:
- name: Core
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixes a few warnings.

@@ -222,8 +222,6 @@ Formal definitions and grammars for CQL2 can be found here:
- [cql.bnf](https://github.com/opengeospatial/ogcapi-features/blob/master/extensions/cql/standard/schema/cql.bnf)
- [cql.json](https://github.com/opengeospatial/ogcapi-features/blob/master/extensions/cql/standard/schema/cql.json)
- [cql.yml](https://github.com/opengeospatial/ogcapi-features/blob/master/extensions/cql/standard/schema/cql.yml)
- A JSON Schema for only the parts of the CQL2 JSON encoding required by this extension is [here](cql.json)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

edited cql.json has been removed, as we no longer have a custom version

@philvarner philvarner changed the title update CQL2 definition to reference OGCAPI yaml file in github update CQL2 definition to reference OGC API yaml file in github Feb 28, 2022
Copy link
Collaborator

@cholmes cholmes left a comment

Choose a reason for hiding this comment

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

This looks good, but seems to also include some various clean-up updates in the config, pr template, gitignore, etc. Ideally those would be a separate PR. I don't think that's necessary, but maybe also mention those in the title of the PR?

@philvarner philvarner changed the title update CQL2 definition to reference OGC API yaml file in github update CQL2 definition to reference OGC API yaml file in github, update npm build Mar 3, 2022
@philvarner philvarner merged commit 5753396 into radiantearth:dev Mar 3, 2022
@philvarner philvarner deleted the update-cql-openapi branch March 3, 2022 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants