Extract warning methods into module #40
Conversation
|
I should probably say we have a patch for core that's cooking shortly. |
| @@ -1,42 +1,38 @@ | |||
| module RSpec | |||
| module Support | |||
| module Warnings | |||
| extend self | |||
There was a problem hiding this comment.
Why extend self? As far as I can see, you're not using any of these methods via RSpec::Support::Warnings.<method_name> and I don't think we want to expand the API like that, anyway.
|
This LGTM to me besides the |
|
What's the motivation for this? We took the explicit step of moving these out of a module when we moved them across, as we felt it wasn't necessary given that all we're going to do is then extend it straight back into the original module. |
|
|
||
| unless respond_to?(:deprecate) |
There was a problem hiding this comment.
I'm assuming this has something to do with the core PR @samphippen mentioned, but currently this can't be merged with this removal, as core has different implementations of these methods and this check is to prevent support overriding core's implementation.
|
@JonRowe there's a pull on core that needs to be merged at the same time |
| end | ||
| end | ||
| subject(:warning_object) { | ||
| Object.new.tap {|o| o.extend(RSpec::Support::Warnings)} |
There was a problem hiding this comment.
We tend to use do/end for multiline blocks.
There was a problem hiding this comment.
Also, could this not just be a plain let?
There was a problem hiding this comment.
It could be, but it is the subject of the test, so named subject makes more sense IMO.
There was a problem hiding this comment.
Except subject is intended for one liners which you're not using, and explicit subject is a bit of a smell, so in general when a vanilla let can be used I favour it.
There was a problem hiding this comment.
I completely disagree. Just because it works for one liners does not mean it shouldn't be used elsewhere. Specifically I think it draws attention to the actual object under test, as opposed to collaborators.
There was a problem hiding this comment.
We do indeed disagree. I prefer to let my docstrings and let names explain what's under test.
There was a problem hiding this comment.
I have to say I agree with @samphippen subject distinguishes the object from the other collaborators. While it isn't absolutely necessary I think it's a nice hint.
|
I'm with the plan now, left a few minor quibbles but otherwise LGTM |
|
This LGTM. |
|
@myronmarston I think we need to merge this and rspec/rspec-core#1253 simultaneously, so I'll hold off merging until that;s done. |
Extract warning methods into module
Pairing with @samphippen
Refactoring warnings for PR on RSpec core rspec/rspec-core#1253