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

Add Draft 7 to 2019-09 upgrade rules #20

Closed
wants to merge 2 commits into from
Closed

Conversation

suprith-hub
Copy link
Collaborator

@jviotti I have added the rules for draft7 to 2019-09 except for ignore adjacent keywords when $ref is present
I have tested the rules.
Please review the PR

@jviotti
Copy link
Member

jviotti commented Jul 17, 2024

Very cool! Let me give it a proper careful look tomorrow

@jviotti jviotti changed the title 7to2019 rules Add Draft 7 to 2019-09 upgrade rules Jul 18, 2024
@jviotti
Copy link
Member

jviotti commented Jul 18, 2024

Two initial overall comments:

  • Some rules have numeric prefixes and some don't? Can we add them to all rules for consistency?
  • I don't think we need to add numeric prefixes to the tests

@jviotti
Copy link
Member

jviotti commented Jul 18, 2024

Also, if we will go for numeric prefixes, I think you should also add them to the 2019-09 => 2020-12 existing rules, so that the entire repo is consistent. Maybe do those as a quick separate PR?

"deprecated": "newbie"
},
"to": {
"x-deprecated": "newbie"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is correct. 2019-09 does have the deprecated keyword: https://www.learnjsonschema.com/2019-09/meta-data/deprecated/

Copy link
Collaborator Author

@suprith-hub suprith-hub Jul 20, 2024

Choose a reason for hiding this comment

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

😐 Draft 7 doesn't have deprecated keyword, 2019 has. So, if the type of deprecated is object or something invalid then it will be invalid in 2019

Copy link
Member

Choose a reason for hiding this comment

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

Ahhhhhhhhh I remember this now. I wonder if we can make the rule names more descriptive, as if I got confused, others might get confused too. Maybe call them i.e. unknown-before-deprecated or something like that?

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'll say unknown-inp-reviosu-draft-{keyword}..

{
"title": "maxContains present in the schema",
"from": {
"maxContains": "newbie"
Copy link
Member

Choose a reason for hiding this comment

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

maxContains is also present in 2019-09: https://www.learnjsonschema.com/2019-09/validation/maxcontains/.

{
"title": "minContains present in the schema",
"from": {
"minContains": "newbie"
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Also present. Maybe you mis-read the spec?

@jviotti
Copy link
Member

jviotti commented Jul 18, 2024

Another big one is that I see a lot of tests removing keywords that in fact do exist in 2019-09. Maybe you mis-read the spec? Can you handle those first? I think it will make the rest of the PR easier to review. You can see all 2019-09 keywords here: https://www.learnjsonschema.com/2019-09/

@suprith-hub
Copy link
Collaborator Author

Another big one is that I see a lot of tests removing keywords that in fact do exist in 2019-09. Maybe you mis-read the spec? Can you handle those first? I think it will make the rest of the PR easier to review. You can see all 2019-09 keywords here: https://www.learnjsonschema.com/2019-09/

Hm.... I think you missed what the rules tries to convey, maybe I should rename the rule title to unknown in previous draft. Which means it should remain unknown in next draft too....

@suprith-hub
Copy link
Collaborator Author

Let me also break the PR 😅

@jviotti
Copy link
Member

jviotti commented Jul 22, 2024

Haha yes. I forgot about that. Maybe let's make the rule names a bit more descriptive so that others don't get confused too? Cool that you are breaking the PR. Probably easy to review. Let me know when you have done so!

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.

2 participants