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

Allow addressable 2.4+ to work with json-schema #312

Merged
merged 1 commit into from
Sep 29, 2016

Conversation

iainbeeston
Copy link
Contributor

I've had to change one of the specs because of this. It seems that
addressable now raises an error if you parse a uri that looks like a
json text, which was one of our test cases.

This fixes and supersedes #301

@iainbeeston iainbeeston changed the title Allowed addressable 2.4+ to work with json-schema Allow addressable 2.4+ to work with json-schema Feb 26, 2016
@iainbeeston
Copy link
Contributor Author

The catch with this change is that addressable 2.4.0 produces a load of ruby warnings (again)

@RST-J
Copy link
Contributor

RST-J commented Feb 26, 2016

👍

@iainbeeston
Copy link
Contributor Author

@RST-J there are two catches with this PR:

  1. It forces everyone to upgrade addressable (because it's become stricter, which means the tests behave differently)
  2. addressable 2.4.0 has an annoying ruby warning (unused variable)

I'd like to fix 2 at some point, but how do you feel about 1?

@RST-J
Copy link
Contributor

RST-J commented Mar 4, 2016

I don't mind to force a dependency to be updated as long it does not silently break other places.
What if a project using json-schema also depends on adressable but in a minor version than 2.4? Is that possible by explicitly stating that in the Gemfile while json-schema uses 2.4?
If so, I'm totally fine with that.

@iainbeeston
Copy link
Contributor Author

I'm afraid that there's no way to do that. If we merge this change, anyone updating json schema would also have to update addressable (throughout their project).

It might be worth holding off on merging this until there is at least addressable 2.4.1 (ie when other gems have had more time to upgrade as well)

@RST-J
Copy link
Contributor

RST-J commented Mar 12, 2016

I mean, as long as it is clear that upgrading json-schema requires upgrading addressable, people may stick with the current version, if they cannot upgrade addressable for whatever reason.
But in general it should be the goal to keep up at least with minor updates, so I think it is Ok if we merge it and have it in the next release.

@matthewrudy
Copy link

I've played with this
and narrowed the warnings down to this section
https://github.com/ruby-json-schema/json-schema/blob/master/lib/json-schema/attributes/ref.rb#L26-L41

In fact I got it to a smaller minimum

a = Addressable::URI.parse('#')
b = Addressable::URI.parse("http://google.com")
a.defer_validation do
  a.merge!(b)
  a.fragment = ""
end

@RST-J
Copy link
Contributor

RST-J commented Jul 27, 2016

@iainbeeston it does not look like there will be 2.4.1 anytime soon. There is only one commit on master since the release of 2.4.0. Anyways, it is out for more than half a year now. I think, it is reasonable to merge this. Although we could wait until we make our next minor release. And maybe you find a way to fix the warning thing until then?

@iainbeeston
Copy link
Contributor Author

Yes, you're right - we should look at doing this. I'd still like to try to resolve that warning first though. I'll look at this again soon

@mchapman17
Copy link

Would be great if you could merge this, I'm currently having dependency issues in a project due to some other gems requiring Addressable >= 2.4.

@iainbeeston
Copy link
Contributor Author

Yes, sorry, addressable 2.4 has been out for a while now...

@iainbeeston
Copy link
Contributor Author

I've submitted a fix for the warning messages to addressable sporkmonger/addressable#244

I've had to change one of the specs because of this. It seems that
addressable now raises an error if you parse a uri that looks like a
json text, which was one of our test cases.

This fixes and supersedes voxpupuli#301
@iainbeeston iainbeeston merged commit a485620 into voxpupuli:master Sep 29, 2016
@iainbeeston iainbeeston deleted the update-addressable branch September 29, 2016 12:22
@iainbeeston
Copy link
Contributor Author

For anyone who is waiting for this - I've just released json-schema 2.7, which requires addressable 2.4+

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