Spec: let should be_nil and should be_a(...) cast the value

[asterite]

A way to solve this forum request

Before this PR we had to do this:

index = "foo".index('o')
index.should_not be_nil
foo[index.not_nil!].should eq('o')

The not_nil! is needed because the compiler has no way to know that the should_not be_nil means that index will not be nil.

With this PR we can now do:

index = "foo".index('o')
index = index.should_not be_nil
foo[index].should eq('o')

Basically, the result of obj.should_not be_nil will return obj as a non-nil object.

The same applies for obj.should be_a(T) and obj.should_not be_a(T).

This still doesn't automatically restrict the var but is significantly better than the old option.

This PR is a breaking change because I introduced the Expectation module that all expectations must include. An alternative to implement this PR is to overload should and should_not with BeNil and BeA but it felt like a worse option and it's less extensible.

This is just a draft because together with this PR we can actually change many should_not be_nil combined with not_nil! in the stdlib specs.

Finally, this PR also finally documents the way to extend spec with custom expectations.

💭 In RSpec they are called "matchers", maybe we could rename them to be consistent with the Ruby world? It's also shorter.

[Fryguy]

Isn't the .not_nil! sufficient by itself? That is, you don't need the should_not be_nil, because not_nil! will raise an exception and fail that example. Similary .as can be used to verify a type and raise a runtime assertion (though admittedly it does casting, which may not be desirable)

require "spec"

context "foo" do
  it "be_nil" do
    x = nil
    x.not_nil!.should eq "x"
  end

  it "be_a" do
    x = rand > 1 ? 1 : nil
    x.as(Int32).should eq 1
  end
end

  1) foo be_nil

       Nil assertion failed (NilAssertionError)
         from src/nil.cr:106:5 in 'not_nil!'
         from foo.cr:6:5 in '->'
         ...


  2) foo be_a

       cast from Nil to Int32 failed, at /Users/jfrey/projects/external/crystal/foo.cr:11:5:11 (TypeCastError)
         ...

[Fryguy]

Finally, this PR also finally documents the way to extend spec with custom expectations.

💭 In RSpec they are called "matchers", maybe we could rename them to be consistent with the Ruby world? It's also shorter.

+1 for naming them Matchers. Calling them expectations is confusing because those already have a meaning in RSpec.

In RSpec, "expectations" are the result of expect(thing) or expect { thing } in the newer expectation syntax, and those can be compared to matchers with the .to and .to_not methods. (e.g. expect(thing).to eq 5)

I actually prefer the expect syntax, personally, and it avoids having to patch methods. @asterite would you be ok with that being added as an alternate spec syntax?

[asterite]

@Fryguy

Isn't the .not_nil! sufficient by itself?

Compare the output:

with not_nil!

1) foo bar

       Nil assertion failed (NilAssertionError)
         from /usr/local/Cellar/crystal/0.31.0_1/src/nil.cr:106:5 in 'not_nil!'
         from foo.cr:6:9 in '->'
         ...

with should_not be_nil

  1) foo bar
     Failure/Error: x = a.should_not be_nil

       Expected: nil not to be nil

     # foo.cr:6

I think should_not be_nil has a much better output. It also points you to the exact location, and it shows you the line that failed the assertion.

[asterite]

@Fryguy

I actually prefer the expect syntax, personally, and it avoids having to patch methods. @asterite would you be ok with that being added as an alternate spec syntax?

No, we already had this discussion in the past and we kind of agreed that should involves less typing and it's easier to read and write.

[RX14]

looks good!

[asterite]

I pushed another commit to make it backwards-compatible with a deprecation message, so now this should be fine to merge.

We can change not_nil! in existing specs to use should_not be_nil in subsequent PRs: there are a lot of them and they are not necessary to merge this PR.

[asterite]

I also want to rename Spec::Expectation to Spec::Matcher, but again, in a separate PR.

[straight-shoota]

I might actually prefer implementing the special cases for BeNil and BeA directly in should/should_not. This would avoid a lot of overhead introduced by cast_should and cast_should_not and be easy and simple to understand.

I don't think we need to care for extensibility here, because I honestly can't think of any other valid use case for a generic solution. And if there was, it could always be changed later without breaking anything.

But we can reduce complexity by going the simple way.

Now, adding Spec::Expectation or Spec::Matcher might be a useful addition on its own. But this could be discussed separately.

[asterite]

I agree, I'll send a simpler PR later.

[straight-shoota]

@asteite Is this still on your radar? Otherwise we should make an issue to track this.

[asterite]

Yes, please make an issue. I'm in the middle of a move and I don't have time for Crystal other than just replying on commenting on issues. Thank you!

[RX14]

I'm fine with merging this PR without simplifying. It's not a big deal one way or the other, and can be changed in the future.

[asterite]

Thanks for reminding me about this, I'll send a PR with the simplified version tomorrow.