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

feat: Use latest ajv #166

Merged
merged 6 commits into from
Nov 18, 2021
Merged

feat: Use latest ajv #166

merged 6 commits into from
Nov 18, 2021

Conversation

l0b0
Copy link
Contributor

@l0b0 l0b0 commented Nov 17, 2021

No description provided.

@l0b0 l0b0 marked this pull request as draft November 17, 2021 19:10
@l0b0 l0b0 marked this pull request as ready for review November 17, 2021 19:35
blacha
blacha previously approved these changes Nov 17, 2021
@amfage amfage requested review from blacha and paulfouquet November 17, 2021 20:11
"contains": {
"const": "https://stac.linz.govt.nz/_STAC_VERSION_/film/schema.json"
}
},
{
"type": "array",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does 'array' need to be specified for each item?

Copy link
Contributor Author

@l0b0 l0b0 Nov 17, 2021

Choose a reason for hiding this comment

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

The docs clarify this:

This is a very common mistake that even people experienced with JSON Schema often make - the problem here is that any value that is not an [array] would be valid against this schema - this is rarely intentional.

Copy link
Contributor Author

@l0b0 l0b0 Nov 17, 2021

Choose a reason for hiding this comment

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

In fact, looking at the output there are a bunch more of these. I'll do those as well, but I've put those in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure i follow the docs here, do you have an example of what would be valid that shouldn't be valid?

Copy link
Contributor Author

@l0b0 l0b0 Nov 17, 2021

Choose a reason for hiding this comment

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

I can't seem to create a test which makes the fixes necessary, so it's possible ajv is just being overzealous. There are a few issues related to how strictTypes works, but I couldn't see one which is strictly equivalent to not being useful. I guess this would just be a "better safe than sorry" change, which we can refine if/when ajv refines their validation.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder why its a lint issue then 🤷 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because, in general, if you don't specify the type any type is valid. If you happen to have a sufficiently loose child schema this might result in weird stuff like making a null valid in place of an object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In any case, this discussion is now relevant for the other PR. What do you think of the current PR?

@l0b0 l0b0 force-pushed the feat/use-latest-ajv branch from 4352e49 to 6a0f934 Compare November 17, 2021 21:57
@l0b0 l0b0 added the automerge kodiak automerge label label Nov 18, 2021
@kodiakhq kodiakhq bot merged commit 889bc08 into master Nov 18, 2021
@kodiakhq kodiakhq bot deleted the feat/use-latest-ajv branch November 18, 2021 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge kodiak automerge label
Development

Successfully merging this pull request may close these issues.

4 participants