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

obj.should != something doesn't work properly #33

Closed
myronmarston opened this Issue Oct 19, 2010 · 10 comments

Comments

Projects
None yet
3 participants
@myronmarston
Copy link
Member

myronmarston commented Oct 19, 2010

A coworker of mine who has never used RSpec but is interested in it pointed me to this code snippet from zenspider:

describe Integer do
  it "sucks" do
    10.should == 10
    10.should != 10
  end
end

This spec passes. I know that the recommended RSpec way is to use should_not == rather than should != but this is definitely very confusing behavior.

I was going to go fix it, but this is such a simple case that I'm surprised it hasn't been addressed before, and it made me wonder if there's a specific reason it's not supported. If we're only going to support should_not == then we should at least give the user an error or warning when they use should != as this is very confusing behavior.

@dchelimsky

This comment has been minimized.

Copy link
Member

dchelimsky commented Oct 19, 2010

Of course it's been addressed before, and it is not quite as simple as you might think. In Ruby 1.8 and lower, == is a method, but != is not:

10.should == 10
#becomes
10.should.==(10)

... whereas ...

10.should != 10
#becomes
!(10.should.==(10))

So RSpec has no way to know that it's wrapped in a negating expression without parsing the file, which is not cheap. And we'd have to do it for every == expression to check whether it is == or !=.

If you can figure out a way to support should != unexpected without paying a performance penalty, you win the prize.

@dchelimsky

This comment has been minimized.

Copy link
Member

dchelimsky commented Oct 19, 2010

BTW - this could be supported in Ruby 1.9 because != is a method. But at this point I won't include it in 1.9 unless we can support it in 1.8 too. That will likely change in the future as >= 1.9 adoption increases.

@myronmarston

This comment has been minimized.

Copy link
Member Author

myronmarston commented Oct 19, 2010

My intuition that this wasn't as simple as appears at first glance was correct :).

I agree 100% with not adding features to RSpec that are only supported on ruby 1.9. But I do think it's unfortunate that it's so easy to write x.should != value and not realize the mistake. It's definitely surprising, and the extent that it violates the POLS, I think we should do what we can to warn users rather than silently ignoring the !.

I'm going to play with this for a bit to see if I can come up with a way to support this on 1.8 (it's a nice challenge!), but, in the meantime, I want to discuss a plan B:

  • On 1.9, I think we should override the operator so that it raises an error, explaining that users should use should_not == instead.
  • On 1.8, we could probably get a similar warning on 1.8 by applying a regex like /should(?:_not)?\s+!=/ to each spec file, and print a warning for any matches. This would not have much of a performance penalty since it's done once for each spec file (maybe when the file is loaded?), rather than doing it during the spec run for each use of should ==. Note that I'm not at all sold on this idea as it feels like a hack, but if it can be done in a clean, maintainable way, it may be worth doing. Alternately, we can decide to do nothing for 1.8. I think the additional code on 1.9 to raise an error is worth having since it would be pretty clean and isn't really a new feature for 1.9--it actually helps you write 1.8/1.9 compatible specs.

Thoughts?

@dchelimsky

This comment has been minimized.

Copy link
Member

dchelimsky commented Oct 19, 2010

Why don't you take some time to try and solve for 1.8 first. I wouldn't want to add the warning in 1.9 one release and then take it back the next release w/ a solution :)

One possibility would be to add an optional extension that only loads if configured to add support for != in 1.8. Then one could knowingly choose this, even if it performs slower. WDYT?

@myronmarston

This comment has been minimized.

Copy link
Member Author

myronmarston commented Oct 19, 2010

So...I've researched this for a couple of hours, and this is extremely non-trivial. It looks doable using ParseTree (or maybe ruby_parser for jRuby support) but it'll be a lot of hard-to-maintain code to get it to work for all cases. For example, someone could do this:

describe MyClass do
  def the_matcher
    foo.should
  end

  it "is valid but non-standard RSpec code" do
    the_matcher != 17
  end
end

Suddenly, reading and parsing the source where the expectation was set becomes much, much harder (not that anyone should write their spec this way...). If we're going to support this, we need to support it consistently. Inconsistent feature support is confusing and unhelpful. I don't think the payoff for this feature is worth the hours of work and maintenance headaches the supporting code will bring.

I really think we're better off giving users appropriate warnings/errors. On 1.9 this is fairly easy since ! is not a method. On 1.8, we can use a regex approach as I suggested above. It won't work consistently in all cases (such as the convoluted hypothetical example above), but that doesn't really bother me--this isn't a feature we're supporting, this is simply a warning for the convenience of the user to guide them away from making an easy and silent mistake.

@dchelimsky

This comment has been minimized.

Copy link
Member

dchelimsky commented Oct 23, 2010

I agree that the warning in 1.9, where it's easy to do, is worthwhile. Feel free to patch.

@myronmarston

This comment has been minimized.

Copy link
Member Author

myronmarston commented Oct 23, 2010

Fix here:

http://github.com/myronmarston/rspec-expectations/tree/not_equals_fix

On a side note, it'd be nice if github would allow me to attach this to this issue to turn it into a pull request (since pull requests create an issue...)

@myronmarston

This comment has been minimized.

Copy link
Member Author

myronmarston commented Nov 6, 2010

Raise an error if should/should_not !=/!~ is used since these operators cannot be supported on ruby 1.8.

@taw

This comment has been minimized.

Copy link

taw commented Aug 27, 2017

That will likely change in the future as >= 1.9 adoption increases.

Well, nobody uses 1.8 any more and a.should == b and a.should != b look far better than any alternative syntax, so maybe time to add them back?

@myronmarston

This comment has been minimized.

Copy link
Member Author

myronmarston commented Aug 28, 2017

@taw that ship has sailed, given the should syntax, while still there, is no longer the "main" syntax, and expect (intentionally) does not support direct operator usage like expect(x).to == y.

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.