Navigation Menu

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

Fix be_nil expectation to use .nil? #5091

Merged
merged 1 commit into from Oct 9, 2017

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Oct 9, 2017

The be_nil expectation compares the actual value with nil using the == operator. This operator can be redefined by any class. This makes it unreliable for testing if a value is actually nil.

I would expect the be_nil expectation to test with .nil? (which as a compiler feature cannot be redefined). be is used of identity while eq tests equality.

If it should matter for any purpose, the current behaviour of the == operator can easily be accomplished by using eq nil instead of be_nil.

@@ -8,7 +8,7 @@ describe "YAML" do
it { YAML.parse_all("---\nfoo\n---\nbar\n").should eq(["foo", "bar"]) }
it { YAML.parse("foo: bar").should eq({"foo" => "bar"}) }
it { YAML.parse("--- []\n").should eq([] of YAML::Type) }
it { YAML.parse("---\n...").should be_nil }
it { YAML.parse("---\n...").should eq nil }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain why it should eq nil and not be nil?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YAML.parse returns a YAML::Any, thus it is not nil but any == nil still returns true if any.value is nil.

@RX14 RX14 merged commit 885eda6 into crystal-lang:master Oct 9, 2017
@RX14 RX14 added this to the Next milestone Oct 9, 2017
@straight-shoota straight-shoota deleted the jm-nil-expectation branch October 9, 2017 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants