Gregsdennis/dynamic ref naming #1656
Conversation
|
@gregsdennis, I actually started working on this a while ago (had to stop to prepare my talk for the conf). I should have communicated that I was work on it. Anyway, it's mostly the same (sometimes word for word) as what I have. The one thing I did different was that I want to move away from referring to dynamic anchors as fragments because fragments are a URI concept. Do you mind if push to this branch some of what I was working on to reflect that change? |
|
@jdesrosiers yeah feel free. I likely missed something anyway. |
| The value of the `$dynamicRef` property MUST be a string and MUST conform to the | ||
| plain name fragment identifier syntax defined in {{fragments}}.[^3] |
There was a problem hiding this comment.
There's no reason for this to be limited to fragment identifier syntax because it's not a fragment identifier anymore. Any objection to changing this to be any string?
There was a problem hiding this comment.
I think keeping it this syntax simplifies schemas. Yeah it'll work with any string, but I don't think it helps necessarily.
There was a problem hiding this comment.
I'm ok with not changing it, but I want to include a comment noting why it's restricted even though there's no technical reason it needs to be like there is with $anchor. Then I realized, I couldn't write that up because I didn't understand your reasoning 😅. So, can you help clarify,
I think keeping it this syntax simplifies schemas.
I'm not sure what you mean. What does it simplify exactly. Removing the restriction simplifies implementations that no longer need to make that check. It also simplifies writing schemas because the schema author doesn't need to know the rules for what's allowed and what isn't. It also simplifies the meta-schema. The only reasons I can think of for keeping it is to maintain consistency with $anchor or to just avoid changing it because the change doesn't add any value.
There was a problem hiding this comment.
I'm less concerned about it being anchor-like and more concerned with it being identifier-like. I think allowing any string (whitespace, special chars, etc.) could get confusing, but admittedly, that's probably the dev's responsibility.
There was a problem hiding this comment.
I see, it's a style thing. Yeah, that's the schema author's concern. We shouldn't need to address that in the spec.
There was a problem hiding this comment.
This name could be used as a function name via code generation, so a limited character set is useful in that case.
There was a problem hiding this comment.
@karenetheridge Could you explain how a dynamic anchor could be used as a function name? I'm not seeing that connection. In any case, I don't think we should be making decisions based on what might be needed for code gen. We certainly need to come up with a code gen spec at some point, but I don't want make some decision now and later find that it was unnecessary or we have to go another direction completely.
There was a problem hiding this comment.
Same as for any anchor -- from my recollection codegen was one of the usecases in mind for the restricted character set for anchors.
At any rate, I see no compelling reason to expand the syntax from what it is now, and it could cause issues.
There was a problem hiding this comment.
Same as for any anchor
Sorry, it's not obvious to me how an anchor maps to a function name. Codegen is typically about generating types. I'm not seeing where functions come in at all, let alone how anchors are involved. Maybe you meant "type names" instead of "function names" and it's about using anchors as a way to name a type?
from my recollection codegen was one of the usecases in mind for the restricted character set for anchors.
I don't recall if that was part of the discussion, but it's not the reason that's given in the spec. The concept has to do with URI fragments and content negotiation. How URI fragments are interpreted is defined by the media type, so it's necessary for media types to standardize on what values an anchor can have so you don't end up with cases where one media type can define an anchor that another one can't and therefore it isn't safe to request one media type in place of another. See, https://json-schema.org/draft/2020-12/json-schema-core#section-5-4 and the spec it references.
Since dynamic anchors can no longer be used in URI fragments, this restriction is no longer relevant. Which is why I suggested removing it. In part, I think removing it would help signal that dynamic anchors are not fragment identifiers.
I see no compelling reason to expand the syntax from what it is now
That's fair. The argument that changing it adds no value is a good one. We can just say it's another one of those things that's like that for "historical reasons". I'm ok with that. I'm just not ok with the reason being codegen right now, not until we have a clear consensus about what codegen should look like for JSON Schema.
jdesrosiers
left a comment
jdesrosiers
left a comment
There was a problem hiding this comment.
I added a note explaining why dynamic anchors are limited to fragment identifier syntax even though they aren't used in URIs. I think this is ready now.
jdesrosiers
force-pushed
the
gregsdennis/dynamic-ref-naming
branch
from
bb54f2d to
92249ec
Compare
|
(the force push was a rebase) |
3 participants
What kind of change does this PR introduce?
Update
Issue & Discussion References
$dynamicAnchorresource identification is confusing #1655Summary
Changes usages of
$dynamicRefto use the same plain-name identifier defined by$dynamicAnchorinstead of the IRI syntax.Does this PR introduce a breaking change?
Yes. The IRI syntax is no longers supported.
Implementations still have the option to deviate from the specification and support IRI-syntax anchors, but this must be disabled. (Do we need to explicitly state this?)