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

test that format-assertions are valid with non-string types #512

Merged
merged 2 commits into from
Sep 26, 2021

Conversation

karenetheridge
Copy link
Member

Also resolves some inconsistencies between draft versions.

@karenetheridge karenetheridge requested review from gregsdennis and a team September 5, 2021 18:19
@Julian
Copy link
Member

Julian commented Sep 6, 2021

These belong in non-optional (they're required no matter what the implementation of format looks like). And I believe are present already? Though I see you saying resolving some inconsistencies -- which ones?

@karenetheridge
Copy link
Member Author

karenetheridge commented Sep 7, 2021

These belong in non-optional (they're required no matter what the implementation of format looks like). And I believe are present already?

The point is to test the evaluation result when formats are treated as an assertion, as well as just an annotation.

Though I see you saying resolving some inconsistencies -- which ones?

They are in their own commit: 809a114?diff=split&w=1

@Julian
Copy link
Member

Julian commented Sep 8, 2021

The point is to test the evaluation result when formats are treated as an assertion, as well as just an annotation.

I suspect the thing I don't follow is the same thing that was just opened as #513, I don't really understand what differentiates one from the other in what's here (which isn't new in this PR, but yeah seems quite confusing at the minute).

@karenetheridge
Copy link
Member Author

format-annotation and format-assertion are two totally separate vocabularies. Non-string types are valid in both vocabularies. We had tests for one but not the other (as observed by @gregdennis on slack a few days back). This PR corrects that.

@karenetheridge karenetheridge force-pushed the ether/formats-and-non-strings branch from 097e6ff to 494676a Compare September 11, 2021 16:09
Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

LGTM (the tests themselves look good)

@ChALkeR
Copy link
Member

ChALkeR commented Sep 12, 2021

That said, I don't understand why are these in optional.

Currently, optional are an addition to non-optional tests, so non-optional should pass in any configuration (either assertion or annotation of formats).

I don't see the logic for duplicating these in optional.

@karenetheridge
Copy link
Member Author

The intent is to ensure that the format-assertion vocabulary also pass these tests.

non-optional should pass in any configuration (either assertion or annotation of formats)

Yes, but that is not actually clear right now -- as was raised by a core contributor whose implementation was not passing these tests (and therefore he was not testing in that configuration). I would rather err on the side of redundant/overlapping tests, than missing some test coverage.

This will all be more clear when we add custom metaschemas -- that is, add a "$schema" keyword to the format-assertion tests.

@karenetheridge karenetheridge force-pushed the ether/formats-and-non-strings branch from 494676a to b349b87 Compare September 26, 2021 23:09
@karenetheridge karenetheridge merged commit 3fcee38 into master Sep 26, 2021
@karenetheridge karenetheridge deleted the ether/formats-and-non-strings branch September 26, 2021 23:10
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