Make ValidateUniquenessOfMatcher method signature consistent

[PikachuEXE]

• Make ValidateUniquenessOfMatcher#allow_nil & #allow_blank accepts an optional boolean argument

This makes it consistent with ValidateInclusionOfMatcher

For #721

[maurogeorge]

@mcmire @PikachuEXE

We really need this optional boolean? If your answers is positive I think we need to add tests and document this. I add this to validate_lenght_of but without the optional param on #724 and #725.

I think the regular use of this qualifier is like:

class Post < ActiveRecord::Base
  validates_uniqueness_of :author_id, allow_nil: true
end

# RSpec
describe Post do
  it { should validate_uniqueness_of(:author_id).allow_nil }
end

But if I do not want to allow nil I can use the negative form like:

class Post < ActiveRecord::Base
  validates_uniqueness_of :author_id
end

# RSpec
describe Post do
  it { should_not validate_uniqueness_of(:author_id).allow_nil }
end

I am missing something? Do you guys have a use case to this?

Thanks

[PikachuEXE]

Ask @mcmire
I am just trying to make them consistent
Besides being able to specify false is an explicit message telling others that
The validation should not allow nil/blank values.

In your example, without optional boolean support,
it would require two examples:

it { should validate_uniqueness_of(:author_id) }
it { should_not validate_uniqueness_of(:author_id).allow_nil }
# Then I might also need
it { should_not validate_uniqueness_of(:author_id).allow_blank }

If this PR is merged, it becomes:

it { should validate_uniqueness_of(:author_id).allow_nil(false).allow_blank(false) }

[maurogeorge]

@PikachuEXE thanks for your feedback.
Let's wait @mcmire answer to see if we will follow with optional param or not. I don't see any problem on have something like your example.

[mcmire]

@PikachuEXE @maurogeorge I think it's fine for all of the boolean qualifiers to take an optional value. It reads better in the true case (allow_nil vs. allow_nil(true)). This trend has existed for a while with some of the other matchers, and I didn't feel like reverting that for 3.0. So I'm just making everything consistent now.

The examples you two have mentioned would work regardless of whether this takes an optional argument. This change wouldn't affect existing tests.

[PikachuEXE]

So...do I need to make more changes to this PR?

[mcmire]

@PikachuEXE Yes, we need to add tests for this.

[PikachuEXE]

I see there is a pre-release version coming out (or came out)
Should I change it before some date?
Is there any similar tests I can "copy and modify" for this PR?

[mcmire]

Actually I just re-read over this and realized that we probably don't need tests for this. Are people going to use allow_blank(false) or allow_nil(false)? That seems a little redundant to me. So, the existing tests seem fine.

The pre-release version already came out, yeah, so I probably won't get to merging this until after that is released.

[maurogeorge]

@mcmire I agree with you that use allow_blank(false) or allow_nil(false) seems a little redundant, I usually use something like: validate_uniqueness_of(:author_id).allow_nil and if my model does not define allow_nil I simple do not add this to my tests. I do not do a should_not validate_uniqueness_of(:author_id).allow_nil or the should validate_uniqueness_of(:author_id).allow_nil(false).

But if us assume that no one will use the allow_blank(false) or allow_nil(false) we can remove the default value and use true directly. What you think?

IMO this optional param is a hidden feature, since this is not documented on RDoc. I think we can embrace this as a feature(adding docs and tests to this) or remove this hidden feature(removing the optional param).

I am not sure about version if this will be a MAJOR or MINOR. I know that will break this API if we remove the optional param, but is this a real API or a hidden feature since this is not on docs.

[mcmire]

IMO this optional param is a hidden feature, since this is not documented on RDoc

It's only optional because in the true case, people leave off the (true) and just say e.g. allow_nil. This case actually is documented. The opposite of that -- allow_value(false) -- isn't documented, but I don't think it matters if no one's ever going to use it.

But if us assume that no one will use the allow_blank(false) or allow_nil(false) we can remove the default value and use true directly. What you think?

If we removed the default value, then we'd be forcing people to change all of their allow_nil's to allow_nil(true), which would be a backwards-incompatible change. I don't think it's worth it to ask people to have to do that -- this has already been a precedent for a long time, and I don't think it's unreasonable that people (especially people that have been using shoulda-matchers for a while) are doing this.

[maurogeorge]

@mcmire following your docs you are right, the example is showed with the default true param like:

  it { should validate_uniqueness_of(:author_id).allow_nil }

I don't mean remove the default param, but remove the param at all. Change the allow_nil to something like:

def allow_nil
  @options[:allow_nil] = true
  self
end

The opposite of that -- allow_value(false) -- isn't documented, but I don't think it matters if no one's ever going to use it.

Since no one will use the param, what you think about remove this?

[mcmire]

Ohh... sorry, I misunderstood. I see what you're saying now.

So on one hand, accepting an explicit argument creates a symmetry between validation options and matcher qualifiers. When I first started working on shoulda-matchers, I had this idea that if a validation takes an option, the corresponding matcher should take a qualifier with the same name. In other words, you should be able to look at your validation and pretty easily guess what the corresponding qualifier on the matcher would be. So if you see allow_nil: true, you should be able to say allow_nil(true). It seems like it would make it easier for newbies to get rolling really quickly.

That said, we don't completely follow this idea right now. There are a few cases where the qualifier is named differently than the validation option (scope vs. scoped_to, greater_than vs. is_greater_than, etc.). Should we try to make this better? I'm not sure. Probably.

But actually... symmetry doesn't matter so much in this case. As a counterpoint, something else to consider is that boolean qualifiers are actually different from normal qualifiers. When you use them, they add an extra test that the matcher will perform -- but only if you're using them in a true context. @PikachuEXE earlier you said this:

Besides being able to specify false is an explicit message telling others that the validation should not allow nil/blank values.

Unless I'm misunderstanding, it sounds like you think that if you say allow_nil(false), you're telling the matcher, test that this validation should not work with nil values. But this isn't actually what the qualifier does. In fact, allow_nil(false) is a no-op -- it does nothing. In pseudocode, the matcher is doing this:

if allow_nil qualifier was provided and is true
  test that the record doesn't fail validation if the value for the attribute under validation is nil
else
  # do nothing
end

Perhaps it should do something meaningful. But the fact is, if your validation doesn't have an allow_nil option on it, you're saying, "I don't care whether this attribute is nil, I assume it will never be nil." So it actually makes sense that allow_nil(false) does nothing -- the matcher will test the validation using a non-nil value, but will never run the matcher against nil.

In light of this last point, I agree with @maurogeorge that we should take off optional parameters for boolean qualifiers across the board (considering it isn't documented anyway), so that people won't get misled about how they work.

[PikachuEXE]

@mcmire
I thought the gem got this already:

if allow_nil qualifier was provided and is true
  test that the record doesn't fail validation if the value for the attribute under validation is nil
else
  test that the record does fail validation if the value for the attribute under validation is nil
end

I guess I am imaging it while reading the code :P
Then I support removing it for now.
It's a new feature to accept false to have an extra test.
I will submit another PR for that afterward.

Since this PR does not actually solve anything, close it when appropriate.

[mcmire]

Yeah, we can't always test that the record fails validation if the attribute is nil, because this is only guaranteed to happen for validates_presence_of. But for other validations, you'll probably get some kind of exception.