I fixed that by simply adding hyperschema validators for draft1 and 2 according to the one already present for draft4. Could that have done any harm? And I have no idea why it is not required for draft3.

Another thing which occurred to me: Because I renamed some test, it failed. That was because that test was overwritten by another one with the same method name. Is there a way we can Minitest have raising an exception when a test method is about to be redefined in a test class?

I don't think minitest or test unit would be able to raise an exception if a test is redefined, because it's just a plain old Ruby object. I imagine the best we can do is to make sure tests are run with warnings enabled. I've just raised a pull request to do that

All sounds fine to me. I'm not sure why any? works but include does not (if it's just an array) but I'm not too concerned

👍

I've just realised why the change from include? to any? works. In the way you have defined any, we check "data == value" (where value is an element in the array). Ruby's Array#include? method checks "value == data". By reversing the order we change which object's == method is called. It looks like whatever data#== is it is not behaving the same as value#==.

Ideally == should be symmetric and the two should be equivalent. Perhaps a better fix to this bug would be to correct data#== (but I have no idea what class data is in this instance)

That seems not to be the cause, because the tests still pass when I use enum.any? { |value| value == data }.
When I started to go for this I also found out that, although enum.include?(data) did not work, calling .include?(data) on a literal definition of enum worked in the debugger session.

Good one @iainbeeston :-), the problem is not the type of the values but of enum which is ArraySet, a custom class aiming for improved lookup-speed which fails in this case because 1.0 is just equal to 1 but not identical.

I don't know what the performance impact for this change is. The current fix would make it obsolete, so we could just go back to use an array here and include? will work fine again.
If its a noteworthy improvement, I would change the creation of the hash in ArraySet.include? to add the integer/float variant for equal values.
@hoxworth can you tell something more on this and suggest which way to follow?

Set is also part of 1.8.7 and doesn't json-schema require at least Ruby 1.8.7 since some releases? So we could use it right away.

But that still wouldn't fix the issue here because a set containing 1 responds with false when asked whether 1.0 is included (and the other way around).
So either way we have to add equal Fixnum/Float values for the other if we use some sort of set.

For the existing array set this could look like this:

class ArraySet < Array
  def include?(obj)
    # On first invocation create a HASH (yeah, yeah) to act as our set given the array values
    if !defined? @array_values
      @array_values = {}
      self.each do |x|
        @array_values[x] = nil
        if x.is_a?(Numeric) && x.to_i == x
          other_numeric = x.class == Float ? x.to_i : x.to_f
          @array_values[other_numeric] = nil
        end
      end
    end
    @array_values.has_key? obj
  end
end

The docs for Set explicitly state how equality is handled:

Equality of elements is determined according to Object#eql? and Object#hash.

1.eql?(1.0) yields false, what btw is how I (intuitively) would expect a set to behave.

Then we should probably submit this contradiction somwhere™. Where is the place for such a thing?

And we need a decision for json-schema for which I'd like to wait what @hoxworth has to say.

In this case I feel like we should treat every number in an enum and all comparisons in the enum as a Float, converting Fixnum to float in both the insert and lookup case. I honestly can't even remember why ArraySet was created instead of Set, and it was only a year ago... I'm perfectly fine with moving towards Set.

@hoxworth, probably because otherwise schema validation fails with the following message?

JSON::Schema::ValidationError: The property '#/properties/a/enum' of type Set did not match the following type: array

@RST-J Ah yes, that is exactly why - ArraySet was introduced to fix that exact bug.

I agree, it should behave like Array and that is what it does now (unless there is another case we haven't figured out yet).

Using a binary search won't work because we cannot order the set with reasonable effort in the general case. The spec says:

Elements in the array MAY be of any type, including null.

So we'd need to come up with a function that assigns equal arguments the same value.
This would imply a lot of potential for flaws (only occurring in their special circumstances) and I'm not sure whether the overhead of this function would eat up the benefits of binary search. Maybe hash would be a solution, but how unique is that?

Do you have a serious concern about my last change, @iainbeeston?

P.S.:
Doing this with hash would be reinventing the wheel (i.e. Set):

The hash value is used along with eql? by the Hash class to determine if two objects reference the same hash key.

I think what you've committed is the best we can do right now. The only
feedback I have on that is:

1. Should we require set in that file? (I assume it's already required
   elsewhere)
2. I'd recommend extracting out the "if is_a?(Fixnum) then to_f" to a
   method (which normalises values) that can be called twice. I always find
   that find of thing makes intent clearer, and makes it obvious that we're
   doing the same thing to all values under conlarison.

Otherwise it's 👍 from me

I didn't find any require statement for Set with grep, I don't know why it worked. But I added a require statement for consistency.
And I agree about extracting a function, done.

FYI I've updated ruby's rdoc for set to make it explicit that it doesn't use == for include? ruby/ruby#831

Looks mostly fine. One question: why do the new hyperschemas include JSON::Validator.register_default_validator(self.new)? As there's only ever going to be one default validator, wouldn't it make sense to set it explicitly?

I didn't spent thoughts here, just copied hyper schema validator for draft4.

I think, generally you are right, the default validator should only be set once and it should be draft4. So the right step is to not do it in the two ne hyper schema validators - I'll remove that - and also not doing it in the hyper-schema-draft 4. I'll prepare another PR to remove that.

I removed all default registrations, also for draft4 in the extra commit.

Good stuff RST-J!

👍