Don't add `describe` to every object. #638

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@myronmarston
Member

myronmarston commented Jun 16, 2012

Instead, make it only available on:

  • The main object--so it can be used at the top level.
  • Modules--so example groups can be declared nested
    within modules, as is the common practice.

Besides this, the only other place we need describe is from within
example groups (so we can nest them), and this is taken care of by
RSpec::Core::ExampleGroup.describe.

I got the idea for this from a recent change in Sinatra that similarly
limits the DSL to just the main object rather than all objects:

sinatra/sinatra@46bdb7d

There is some risk to this change, since it is removing describe from lots of objects it was available on previously, but users shouldn't be calling #describe on any object willy-nilly, anyway. The rspec-expectations and rspec-mocks specs pass with this change (as well as the rspec-core specs, of course).

Thoughts?

/cc @dchelimsky @patmaddox @alindeman @justinko @spicycode

Don't add `describe` to every object.
Instead, make it only available on:
  - The main object--so it can be used at the top level.
  - Modules--so example groups can be declared nested
    within modules, as is the common practice.

Besides this, the only other place we need describe is from within
example groups (so we can nest them), and this is taken care of by
RSpec::Core::ExampleGroup.describe.

I got the idea for this from a recent change in Sinatra that similarly
limits the DSL to just the main object rather than all objects:

sinatra/sinatra@46bdb7d
@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Jun 16, 2012

This pull request passes (merged b45c10e into 18903e1).

This pull request passes (merged b45c10e into 18903e1).

@@ -21,4 +21,9 @@ def describe(*args, &example_group_block)
end
end
-include RSpec::Core::DSL
+# make describe available on the main object, but not all objects

This comment has been minimized.

Show comment Hide comment
@dchelimsky

dchelimsky Jun 16, 2012

Member

We don't need these comments since they're described in dsl_spec.rb.

@dchelimsky

dchelimsky Jun 16, 2012

Member

We don't need these comments since they're described in dsl_spec.rb.

@@ -863,6 +863,9 @@ def metadata_hash(*args)
end
end
+ extend Forwardable
+ def_delegators "RSpec::Core::ExampleGroup", :describe

This comment has been minimized.

Show comment Hide comment
@dchelimsky

dchelimsky Jun 16, 2012

Member

This feels like a hack just to get the examples to pass. How about using ExampleGroup.describe in the examples (and get rid of RSpec::Core in some of the other cases in the same file).

@dchelimsky

dchelimsky Jun 16, 2012

Member

This feels like a hack just to get the examples to pass. How about using ExampleGroup.describe in the examples (and get rid of RSpec::Core in some of the other cases in the same file).

@dchelimsky

This comment has been minimized.

Show comment Hide comment
@dchelimsky

dchelimsky Jun 16, 2012

Member

Beside my comments on some details I am wildly in favor of this. Thanks, @myronmarston!

Member

dchelimsky commented Jun 16, 2012

Beside my comments on some details I am wildly in favor of this. Thanks, @myronmarston!

@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Jun 16, 2012

Member

I made your suggested changes, amended my commit with them and pushed to master:

de01e05

On a side note, with this change and the recent change in rspec-expectations, we've eliminated 3 methods that used to be added to every object by rspec: describe, should and should_not, without losing any functionality. rspec-mocks still adds a bunch (stub, stub!, should_receive, should_not_receive come to mind...) and it'd be interesting to see if we could come up with improvements there that keep the functionality without globally monkey patching every object. It'd be pretty sweet if rspec-3 could be (almost) monkey-patch free!

Member

myronmarston commented Jun 16, 2012

I made your suggested changes, amended my commit with them and pushed to master:

de01e05

On a side note, with this change and the recent change in rspec-expectations, we've eliminated 3 methods that used to be added to every object by rspec: describe, should and should_not, without losing any functionality. rspec-mocks still adds a bunch (stub, stub!, should_receive, should_not_receive come to mind...) and it'd be interesting to see if we could come up with improvements there that keep the functionality without globally monkey patching every object. It'd be pretty sweet if rspec-3 could be (almost) monkey-patch free!

@myronmarston myronmarston referenced this pull request in rspec/rspec-mocks Jun 18, 2012

Closed

Use the new expectation syntax for mocks #153

@justinko

This comment has been minimized.

Show comment Hide comment
@justinko

justinko Jun 18, 2012

Contributor

Nice change!

Contributor

justinko commented Jun 18, 2012

Nice change!

@soulcutter

This comment has been minimized.

Show comment Hide comment
@soulcutter

soulcutter Jun 19, 2012

Member

I really like this one - thanks @myronmarston

Member

soulcutter commented Jun 19, 2012

I really like this one - thanks @myronmarston

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment