-
-
Notifications
You must be signed in to change notification settings - Fork 307
Gregsdennis/improve scopes #1588
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
base: main
Are you sure you want to change the base?
Conversation
I still have some work to do here for |
Just did a close/open to rekick the build. It had been a while and couldn't do it manually. |
Reviewed the changes in #1589, and they seem to align with the rest of what I've updated here, so I'm going to leave |
8f29265
to
767debd
Compare
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 an improvement, but I still really dislike the way dynamic scope is defined. I think it should be the stack of schema resources and not include subschemas. The extra complexity makes learning how dynamic references work more complicated.
Also, I desperately hope we wouldn't ever add another keyword that uses dynamic scope. My preference would be to remove this whole concept from the spec and address it only in the context of dynamic references and recommend that extension keywords not use dynamic scope.
All of that can be debated elsewhere and done as a follow up if there's consensus.
I agree, and this is already how we define dynamic scope (unless you're intending to change that?) -- we say elsewhere in the spec, when processing Therefore, in my implementation, the current URI is pushed to the I can't see any usecases for wanting other, non-resource, subschemas to be stored in the dynamic scope. |
I've updated to use resources. (This is actually what we agreed on when @jviotti was writing his posts on scopes.) It does raise one question, though: what if evaluation starts at a subschema? I suppose the dynamic scope is still the resource, but the evaluation path would still start from the subschema? This is how I have it now, and I could change it if we decide something else.
I don't know. I think if explained properly (which is what this change is for), dynamic scopes could be utilized by others that need it. I think it's a powerful concept and it gives extensions another trick. |
What kind of change does this PR introduce?
clarification
Issue & Discussion References
Summary
Improve readability, definitions, and explanations of lexical and dynamic scopes.
Does this PR introduce a breaking change?
No.
There is a
MUST
requirement that was previously listed as undefined behavior before, but thisMUST
was stated in the scopes section anyway. It's just nowMUST
in both places.