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

Fix for relative ref with fragment identifier on Windows #109

Merged
merged 3 commits into from
Oct 30, 2014

Conversation

jlblcc
Copy link
Contributor

@jlblcc jlblcc commented Jul 25, 2014

Schemas like this won't work on Windows:

{
  "$schema": "http://json-schema.org/draft-04/schema#",
  "properties": {
    "a": {
      "$ref": "definition_schema.json#/definitions/foo"
    }
  }
}

Relative links always have # appended. From what I can tell, since the path isn't in the format xxx://, it's passed on to Kernel#open as is - with the trailing #.

Adding chomp('#') fixes the issues. I don't see any problems causes by the addition. All tests pass(new ones added to test for relative refs with fragments) on Win and Linux.

@RST-J
Copy link
Contributor

RST-J commented Oct 28, 2014

Any reason for not considering this?

@pd
Copy link
Contributor

pd commented Oct 29, 2014

👍

I briefly used a friend's windows machine and was able to verify the bug and that this fixes test runs on windows.

pd added a commit that referenced this pull request Oct 30, 2014
Fix for relative ref with fragment identifier on Windows
@pd pd merged commit e20175d into voxpupuli:master Oct 30, 2014
@iainbeeston
Copy link
Contributor

Sorry, I didn't really look at this until now. Can anyone explain what this is actually doing? I find it strange that it's a fix for relative schemas, but the actual change is outside of the if uri.relative? block, and so it affects all schema loads. In fact, inside the relative block, we already remove the fragment.

I also can't help wondering if the real problem is with the URI::File class, and how that loads from a file (perhaps when URI::File opens a new file, should it remove the fragment at that point?)

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.

4 participants