-
-
Notifications
You must be signed in to change notification settings - Fork 219
add tests for $dynamicRef skipping over resources #721
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
I've run these against my implementation, and they pass. |
I haven't yet made time to understand the discussion in #719 to form an opinion, but is this ready for a PR? Specifically it sounded like Ben and at one point yourself were saying part of this behavior is undefined in the spec. Is it or is it not? If it is, we should not test it -- and if it is I think Ben should confirm he agrees, but just sending a PR seems to ignore that comment. That aside, until I actually understand what the discussion is about, is this example really minimized? It seems unlikely that's the case, no? Specifically whatever the subject of discussion is, it doesn't have anything to do with
not raise whatever same question as that issue? An even better one presumably would remove the I'll try to understand what's happening there, and obviously things aren't blocked on me so if others feel this is specified by the spec and a minimal, good test, feel free to LGTM. |
For the required tests in this PR ( Let's definitely make sure we're all on the same page first, but this PR is exactly where I think this should end up (although, yes, it could be simplified a little). |
Happy to accept simplifications, but this is the smallest I could come up with. |
This PR fails in my implementation due to a duplicate resource uri |
@karenetheridge try again, please. |
much better; thanks! |
I feel like we agreed on the other issue this is the most sensible/intuitive thing to do, but I don't think the specification actually says this or specifies any particular handling of this case. |
Resolves #719