voxpupuli / json-schema

Ruby JSON Schema Validator
MIT License
1.53k stars 242 forks source link

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

Closed iainbeeston closed 7 years ago

iainbeeston commented 8 years ago

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 commented 8 years ago

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

RST-J commented 8 years ago

:+1:

iainbeeston commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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 commented 7 years ago

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 commented 7 years ago

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

iainbeeston commented 7 years ago

I've submitted a fix for the warning messages to addressable https://github.com/sporkmonger/addressable/pull/244

iainbeeston commented 7 years ago

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