-
-
Notifications
You must be signed in to change notification settings - Fork 345
Clarify various things about canonical URIs #1104
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a couple suggestions, but I think this cleans up a lot of the concerns I had about how the term canonical is used in the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. I think this is a big improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question but otherwise looks good!
Given the current absense of @handrews, I'll look to create a new superseding PR using these commits, but with one additional change as per the above not yet resolved comment thread.
I've re-evaluated my position and thinking on this, and I believe it's now more aligned with what Henry put in his first comment on this PR. I THINK the associated issue could be resolved with a much smaller set of changes, so will proceed to try that.
Remember this issue is just intended to be minimal clean up to fix glaring problems. It doesn't have to solve everything.
Remember this issue is just intended to be minimal clean up to fix glaring problems. It doesn't have to solve everything.
Indeed. I'm going to propose a super minimal change (far smaller than this) based on my comment from yesterday.
Why was this closed? Even if the goal is to remove the "canonical" language in the next release, I'd still like to have these fixes for the patch release if possible. This fixes the vast majority of the problems with the way "canonical" is used. If we didn't already have this, I'd say don't bother if the goal is to remove "canonical" anyway. But, the work is already done and it's an improvement over what we have, so why not use it?
OK. Reopened.
I closed it because it seems like there are outstanding questions which we aren't going to get answered, and I feel my alternate PR does enough for now to fix the immediate and biggest problem.
I'll re-evaluate this PR with new eyes.
How would you like to proceed? Make a new PR, cerrypicking those commits, and make additional commits to form the PR?
it seems like there are outstanding questions which we aren't going to get answered
What questions are you referring to? We don't have to solve everything for the patch release. If anything is controversial, we don't need to change it now.
How would you like to proceed?
Merge it as it is. As it is now, it addresses problems and doesn't introduce any new problems. Any additional things we feel needs to be addressed for the patch release can be a separate PR.
jsonschema-core.xml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the purpose is of this paragraph. What does "embed schemas in another format" mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is referring to something like a JSON Schema in an OpenAPI document.
0f6ade1 to
371dae1
Compare
Hey all. I screwed up, and force-pushed to try undo a merge commit (which happened when trying to resolve a conflict using the GitHub UI... mistake!).
I now do not have the permissions to force-push and re-introduce the commits.
As such, I'll do so on a new branch and create a new PR, referencing this one.
I'll then resolve the conflicts and merge, as there were three approvals on this PR.
I've filed a ticket with GH, but it is non-blocking so I guess they will get back to me "sometime".
In fairness, I've seen reasonable response times when I've filed bugs with their ticketing system!
I now do not have the permissions to force-push and re-introduce the commits.
You have admin privileges on the repository, so you can temporarily give yourself permission to force-push master and then remove the privileges again. (don't tell anyone but it's been known to happen before.) ;)
Fixes issue #937, clarifying a number of other things along the way.
While it touches a fair number of lines, I'm fairly sure that it
doesn't anything about conformance.
[NOTE: This does not address the question of whether "canonical" is what we want - that is too large of a change for a patch release which is what this PR is targeted for. That topic should get its own issue if someone wants to propose something about it. It also does not deal with #1059, although it clarifies some things based on discussions there.]
After spending more time reading various writings on the concept
of the "canonical" URI for a resource, and reviewing our language,
I came to the following conclusions:
As part of this, I fixed an outright incorrect statement that
identifier keywords set canonical URIs. Since there is only
one canonical URI and a single schema object could contain
three ($id, $anchor, $dynamicAnchor) or more identifier keywords,
this statement is clearly a bug. These keywords assign URIs,
but only $id assigns a canonical one.
I revamped a lot of wording in descriptions and examples to
hopefully be more precise. I separated the discussion of
the empty fragment in $id from the main paragraph of its
functionality, and clarified that this is talking about a
media-type-specific semantic equivalence, and is not asserting
that RFC 3986 normalization applies to fragments (this has
been a point of confusion).
If someone wants me to split this up and sees a way to do it, I would be happy to do that.