rspec / rspec-expectations

Provides a readable API to express expected outcomes of a code example
https://rspec.info
MIT License
1.26k stars 397 forks source link

Deprecation request: RSpec::Matchers#be with no arguments #761

Open tom-lord opened 9 years ago

tom-lord commented 9 years ago

https://github.com/rspec/rspec-expectations/blob/master/lib/rspec/matchers.rb#L320-L322 https://github.com/rspec/rspec-expectations/blob/master/lib/rspec/matchers/built_in/be.rb#L104-L106

I think that the following:

expect(foo).to be

Should be deprecated, in favour of:

expect(foo).to be_truthy

The existence of the former has just caused me some pain, since someone had written the following test:

expect(json.size).to be      ## (they forgot to specify a number!!!)

- And the test was silently passing, despite testing nothing of any use! (They could have used eq rather than be, which would have immediately highlighted the problem, but oh well.)

Is there a genuine use case I've not thought of, or can we get rid of this argument-free signature of the method?

myronmarston commented 9 years ago

I've personally never been a fan of be w/ no args. I think I'd be ok deprecating it (particularly if more people say they are in favor of this), but after going through the 2.14 -> 2.99 -> 3.0 release sequence and getting user feedback about the way we did deprecations, we're generally planning to hold off on issuing deprecation warnings until 3.99. That's a long ways off so there's nothing to do here yet.

What do other @rspec/rspec devs think about this?

fables-tales commented 9 years ago

Strongly in favour of deprecation/removal of be with no args.

soulcutter commented 9 years ago

I'm also in favor of deprecating this. truthy seems like the way to go now.

myronmarston commented 9 years ago

I mentioned above that we need to hold off on issuing the deprecation warning until 3.99, which is true, but in the meantime we can update the docs to mention that this is deprecated.

@tom-lord -- want to take a stab at that?

tom-lord commented 9 years ago

@myronmarston sure, I'll put something together tomorrow :)

JonRowe commented 9 years ago

I'm on board with this, however I'm pondering if we should deprecate this in code, but have a config option that enables those deprecations e.g. config.verbose_deprecation which defaults to false, so that all we do in 3.99 is turn this to true by default for that release..

myronmarston commented 9 years ago

I'm on board with this, however I'm pondering if we should deprecate this in code, but have a config option that enables those deprecations e.g. config.verbose_deprecation which defaults to false, so that all we do in 3.99 is turn this to true by default for that release..

I think I'd be against adding that config -- after all, we already have 2 config options for deprecations:

Adding a 3rd way of controlling deprecation verbosity is overkill and confusing, IMO.

JonRowe commented 9 years ago

I was more thinking it'd be for our own benefit, we can work on deprecating things without bothering users until 3.99

fables-tales commented 9 years ago

Agree with Myron.

cupakromer commented 9 years ago

I agree with deprecating be. I'm on the fence about providing a method to show deprecations early.

myronmarston commented 9 years ago

I was more thinking it'd be for our own benefit, we can work on deprecating things without bothering users until 3.99

Honestly, seems like less work for us to simply punt on issuing deprecation warnings for now. The YARD doc will have a @deprecated tag so when 3.99 comes around it'll be easy enough to simply search for all instances of deprecated to find what needs a warning.