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

Revert changes to dynamic scope regarding dependence on instance #1053

Conversation

jdesrosiers
Copy link
Member

Since this is mostly reverting, I added jdesrosiers@321d384 to show what the actual diff is from the original spec. It's mostly typo fixes and removing a couple crefs.

The original PR was #1041

@jdesrosiers
Copy link
Member Author

Like I said in #1041, if either of those crefs makes sense to anyone, we can fix/clarify them and put them back in.

Copy link
Member

@karenetheridge karenetheridge left a comment

Choose a reason for hiding this comment

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

I did git diff 3cf7cb8 upstream/pull/1053* to compare the changes (there weren't too many other commits that went in in the meantime so they're easy to mentally filter out).

I'd keep the second CREF, regarding the initial and final URI fragment, but I don't feel super strongly about it (and haven't looked too closely yet at the proposed changes that remove the requirement for two anchors). The first CREF doesn't seem applicable anymore.


* with this entry in my .git/config:

[remote "upstream"]
    url = [email protected]:json-schema-org/json-schema-spec.git
    fetch = +refs/heads/*:refs/remotes/upstream/*
    fetch = +refs/pull/*/head:refs/remotes/upstream/pull/*

@Relequestual
Copy link
Member

Good spot @karenetheridge
@jdesrosiers can you add that back in please? Then it looks like we're good to merge! =]

@jdesrosiers
Copy link
Member Author

jdesrosiers commented Dec 6, 2020

I think there's some miscommunication here. I don't think including that cref as it is is an option. Right now it doesn't make sense. We can leave it in, but if we do we need to fix/clarify it. I've called this out multiple times now and no one has come forward with an explanation or even indicated that they know what it means. If no one understands how this cref makes sense, I think the only option is to leave it out. If we publish that and someone comes around asking what that cref means, we'd have to tell them, ¯\_(ツ)_/¯ nobody knows ¯\_(ツ)_/¯.

@jdesrosiers
Copy link
Member Author

Here's the cref that was removed and my comment from the previous PR.

Requiring both the initial and final URI fragment to be defined
by "$dynamicAnchor" ensures that the more common "$anchor"
never unexpectedly changes the dynamic resolution process
due to a naming conflict across resources. Users of
"$dynamicAnchor" are expected to be aware of the possibility
of such name collisions, while users of "$anchor" are not.

I removed this cref because it appears to be wrong. There's no way that an $anchor can change dynamic scope resolution and even if it could, it's not clear how bookending could possibly solve that problem. If anyone knows what this means, please share and I'll put it back in.

@karenetheridge
Copy link
Member

@handrews added that CREF in commit 940586a; perhaps he can comment?

But we can certainly sort this out next year when we start looking at revisions for the next draft.

@Relequestual
Copy link
Member

Mmm I'm of two minds on this, but it as we can't confidently justify it, let's leave it removed, and work out the original intent later.

@Relequestual
Copy link
Member

@karenetheridge Please do file an issue to review this referencing this PR =]

@Relequestual Relequestual merged commit 593bf0b into json-schema-org:master Dec 7, 2020
@jdesrosiers
Copy link
Member Author

I'll file the issue, I was going to suggest that approach as well.

@jdesrosiers jdesrosiers deleted the revert-dynamic-scope-changes branch July 8, 2022 15:54
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.

3 participants