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.

Sign up for GitHub

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

Jump to bottom

[299, 300] Add tests for invalid use of, and valid use of empty, fragments in "$id" #341

Merged
merged 3 commits into from
May 31, 2020
Merged

[299, 300] Add tests for invalid use of, and valid use of empty, fragments in "$id" #341

merged 3 commits into from
May 31, 2020

Conversation

ssilverman
Copy link
Member

@ssilverman ssilverman commented Apr 25, 2020 •
edited
Loading

Closes #299, closes #300

@ssilverman ssilverman requested a review from a team April 25, 2020 02:17
@ssilverman ssilverman changed the title [299] Add tests for invalid use of fragments in "$id" [299, 300] Add tests for invalid use of fragments in "$id" Apr 25, 2020
@ssilverman ssilverman changed the title [299, 300] Add tests for invalid use of fragments in "$id" [299, 300] Add tests for invalid use of, and valid use of empty, fragments in "$id" Apr 25, 2020
Julian
Julian reviewed Apr 27, 2020
View reviewed changes
tests/draft2019-09/ref.json Outdated Show resolved Hide resolved
@ssilverman ssilverman force-pushed the no-id-frags branch 2 times, most recently from 07b2cf5 to f492315 Compare April 29, 2020 05:30
@ssilverman ssilverman requested a review from Julian April 29, 2020 05:30
@ssilverman ssilverman force-pushed the no-id-frags branch 2 times, most recently from a562977 to 26f17dd Compare April 29, 2020 11:10
@ssilverman ssilverman requested a review from a team May 2, 2020 16:28
@Julian
Copy link
Member

Julian commented May 10, 2020

Left a few comments. Also from glancing at the relevant section in the spec, I see:

If present, the value for this keyword MUST be a string, and MUST represent a valid URI-reference. This URI-reference SHOULD be normalized,

We probably then need a test for the SHOULD there (that an unnormalized URI-reference is still strictly speaking OK -- but if you don't want to address in this PR, obviously feel free to just add an issue for that if one doesn't already exist)

@ssilverman ssilverman force-pushed the no-id-frags branch 3 times, most recently from 666a512 to ebec214 Compare May 11, 2020 18:53
@ssilverman
Copy link
Member Author

@Julian I added some unnormalized tests.

karenetheridge
karenetheridge reviewed May 27, 2020
View reviewed changes
tests/draft2019-09/core/id.json Outdated Show resolved Hide resolved
tests/draft2019-09/core/id.json Outdated Show resolved Hide resolved
tests/draft2019-09/ref.json Outdated Show resolved Hide resolved
@ssilverman ssilverman force-pushed the no-id-frags branch 3 times, most recently from 3b13d19 to 162225e Compare May 27, 2020 18:00
@ssilverman ssilverman requested a review from Julian May 27, 2020 18:01
@ssilverman
Copy link
Member Author

ssilverman commented May 31, 2020 •
edited
Loading

@Julian Just so it's documented, the following isn't possible to test with the current meta-schema (and per our discussion):

We probably then need a test for the SHOULD there (that an unnormalized URI-reference is still strictly speaking OK -- but if you don't want to address in this PR, obviously feel free to just add an issue for that if one doesn't already exist)

@ssilverman
Copy link
Member Author

Force-pushed to track changes in master.

Julian
Julian approved these changes May 31, 2020
View reviewed changes
Copy link
Member

@Julian Julian left a comment

Choose a reason for hiding this comment

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

lgtm!

@ssilverman ssilverman merged commit 7555d41 into json-schema-org:master May 31, 2020
@ssilverman ssilverman deleted the no-id-frags branch May 31, 2020 16:52
@karenetheridge karenetheridge mentioned this pull request Jun 19, 2020
Open
2 tasks
@notEthan notEthan mentioned this pull request Feb 7, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karenetheridge karenetheridge karenetheridge left review comments

@Julian Julian Julian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test for valid use of empty fragment in "$id" Test for invalid use of fragments in "$id"
3 participants
@ssilverman @Julian @karenetheridge