Removed strict $schema: const, so that https also gets transformed #187
Conversation
Signed-off-by: Suprith KG <suprith7kg@gmail.com>
Signed-off-by: Suprith KG <suprith7kg@gmail.com>
Signed-off-by: Suprith KG <suprith7kg@gmail.com>
Signed-off-by: Suprith KG <suprith7kg@gmail.com>
Signed-off-by: Suprith KG <suprith7kg@gmail.com>
Signed-off-by: Suprith KG <77973415+Era-cell@users.noreply.github.com>
|
I know the http URLs do a redirection to the https variant, but I'm not sure if taking those into account is purely valid, as their |
|
Asked here: https://json-schema.slack.com/archives/CT8QRGTK5/p1711651006084029. I believe it is not valid, but I might be wrong. From my understanding, the fact that the official URLs redirect or not is orthogonal to the actual identifiers and its validity might be questionable. |
| @@ -10,7 +10,8 @@ | |||
| "required": [ "$schema" ], | |||
| "properties": { | |||
| "$schema": { | |||
| "const": "https://json-schema.org/draft/2019-09/schema" | |||
| "type":"string", | |||
There was a problem hiding this comment.
| "type":"string", | |
| "type": "string", |
Let's be consistent with spacing after : here and in the other rules
| @@ -10,7 +10,8 @@ | |||
| "required": [ "$schema" ], | |||
| "properties": { | |||
| "$schema": { | |||
| "const": "https://json-schema.org/draft/2019-09/schema" | |||
| "type":"string", | |||
| "pattern": "http[s]?://json-schema.org/draft/2019-09/schema[#]?" | |||
There was a problem hiding this comment.
This pattern matches the given URL in a wider string. You should limit the regex with ^ and $
There was a problem hiding this comment.
Yeah, correct 😅 I missed this
| @@ -62,7 +73,9 @@ | |||
| "$schema": "https://json-schema.org/draft/2020-12/schema", | |||
| "$id": "https://github.com/sourcemeta/alterschema/rules/jsonschema-draft7-to-2019-09/definitions", | |||
| "type": "object", | |||
| "required": [ "definitions" ], | |||
| "required": [ | |||
There was a problem hiding this comment.
All of these look like irrelevant formatting changes. Can we omit them from this PR? If we want to reformat, let's do it on a separate PR purely dedicated to that
There was a problem hiding this comment.
You should probably add a few tests exercising these new rules if we want to get them in
There was a problem hiding this comment.
Alright, my auto-formatter does this thing in VS Code. I can add the tests.
But these tests are validating syntax, but meta schema validation. So, should that be included under standard test-suite?
There was a problem hiding this comment.
Yeah, I mean just upgrade tests that confirm that if you pass the HTTPS URLs they get upgraded too, etc. No actual meta-schema validation needs to take place
|
So it is not technically invalid assuming implementations fetch schemas over HTTP, but it's also not 100% correct and there will be implementations that do not support it. In any case, I guess it doesn't hurt covering this case for implementations that do work in that way 👍 |
I am thinking for such cases where it depends on implementations, it is better to inform the user by providing a warning(also in website will be cooler) or so, because the alterschema maybe isolated from implementations. |
|
Yep, agreed on the warning. Because Alterschema is not just the web app, you can note on your proposal a way for rules to echo back "warnings" to the caller than we can bubble up somehow |
Signed-off-by: Suprith KG <suprith7kg@gmail.com>
…ith2 Signed-off-by: Suprith KG <suprith7kg@gmail.com>
|
@jviotti changes have been done, can you review the PR... |
In the previous rules latest dialects had to use https and old ones http, while both redirect to valid meta-schema's.
I wasted time not knowing why $schema: https://json-schema.org/draft/draft-07/schema had strict restrictions.
So, I am making a PR on this. Please review my PR @jviotti