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

Alias example group #1236

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@michihuber
Contributor

michihuber commented Dec 30, 2013

allows adding custom aliases for example_group through the configuration

this is very much work in progress but I'll be offline for a bit, so maybe somebody can take a first look in the meantime
/cc @JonRowe @myronmarston

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jan 1, 2014

Member

Hey @michihuber -- the build is erroring because of some recent travis/bundler/rubygems changes that broke some things. Can you rebase against master? That should pull in .travis.yml changes that will fix the the install errors.

Member

myronmarston commented Jan 1, 2014

Hey @michihuber -- the build is erroring because of some recent travis/bundler/rubygems changes that broke some things. Can you rebase against master? That should pull in .travis.yml changes that will fix the the install errors.

module Core
# DSL defines methods to group examples, most notably `describe`,
# and exposes them as class methods of {RSpec}.
# They can also be exposed globally (on main and Module) through

This comment has been minimized.

@myronmarston

myronmarston Jan 1, 2014

Member

I think this line should be:

# They can be exposed globally (on `main` and instances of `Module`) through
@myronmarston

myronmarston Jan 1, 2014

Member

I think this line should be:

# They can be exposed globally (on `main` and instances of `Module`) through
# it "does something" do
# end
# end
expose_example_group_alias(:example_group)

This comment has been minimized.

@myronmarston

myronmarston Jan 1, 2014

Member

This line is pretty odd...you are exposing example_group as an alias of example_group?

I'm a bit on the fence about whether or not it's a good thing to add example_group as a new method to the DSL. Thoughts from others on that?

@myronmarston

myronmarston Jan 1, 2014

Member

This line is pretty odd...you are exposing example_group as an alias of example_group?

I'm a bit on the fence about whether or not it's a good thing to add example_group as a new method to the DSL. Thoughts from others on that?

This comment has been minimized.

@JonRowe

JonRowe Jan 3, 2014

Member

It's pretty odd, personally I like the idea of it being example group internally, then use this mechanism to expose the DSL as describe and context. I personally don't think we should add example_group to the top level DSL, but this should easily allow those that wish to expose it that way can do so.

@JonRowe

JonRowe Jan 3, 2014

Member

It's pretty odd, personally I like the idea of it being example group internally, then use this mechanism to expose the DSL as describe and context. I personally don't think we should add example_group to the top level DSL, but this should easily allow those that wish to expose it that way can do so.

example_group_aliases << name
(class << RSpec; self; end).send(:define_method, name) do |*args, &example_group_block|
RSpec::Core::ExampleGroup.send(name, *args, &example_group_block).register

This comment has been minimized.

@myronmarston

myronmarston Jan 1, 2014

Member

Please use __send__ rather than send. While send works fine here, elsewhere we've had to use __send__ to deal with objects that have redefined send (e.g. Email#send), and it's good to be consistent and use __send__ everywhere -- that way we're less likely to forget to use it for places where it's important.

@myronmarston

myronmarston Jan 1, 2014

Member

Please use __send__ rather than send. While send works fine here, elsewhere we've had to use __send__ to deal with objects that have redefined send (e.g. Email#send), and it's good to be consistent and use __send__ everywhere -- that way we're less likely to forget to use it for places where it's important.

example_group_aliases.each do |method_name|
to_define = proc do
send(:define_method, method_name) do |*args, &block|
::RSpec.send(method_name, *args, &block)

This comment has been minimized.

@myronmarston

myronmarston Jan 1, 2014

Member

Same here; __send__ rather than send, please.

@myronmarston

myronmarston Jan 1, 2014

Member

Same here; __send__ rather than send, please.

@@ -62,5 +99,5 @@ def self.remove_globally!
end
end
# cature main without an eval
# capture main without an eval

This comment has been minimized.

@myronmarston

myronmarston Jan 1, 2014

Member

Thanks for correcting this typo :).

@myronmarston

myronmarston Jan 1, 2014

Member

Thanks for correcting this typo :).

@@ -124,13 +124,21 @@ def alias_example_to name, extra={}
(class << self; self; end).define_example_method name, extra
end
def alias_example_group_to(name, metadata={})
(class << self; self; end).send(:define_method, name) do |*args, &block|

This comment has been minimized.

@myronmarston

myronmarston Jan 1, 2014

Member

Same here: __send__ please.

@myronmarston

myronmarston Jan 1, 2014

Member

Same here: __send__ please.

def alias_example_group_to(name, metadata={})
(class << self; self; end).send(:define_method, name) do |*args, &block|
combined_metadata = metadata.dup
combined_metadata.merge!(args.pop) if args.last.is_a? Hash

This comment has been minimized.

@myronmarston

myronmarston Jan 1, 2014

Member

This does handle symbols-as-metadata (with true values) properly. Here's how we do it in alias_example_to:

options = Metadata.build_hash_from(args)
options.update(metadata)

I think it would make sense to do the same here. It'll need a spec as well, as I think it's important behavior.

@myronmarston

myronmarston Jan 1, 2014

Member

This does handle symbols-as-metadata (with true values) properly. Here's how we do it in alias_example_to:

options = Metadata.build_hash_from(args)
options.update(metadata)

I think it would make sense to do the same here. It'll need a spec as well, as I think it's important behavior.

@@ -242,7 +250,8 @@ def self.describe(*args, &example_group_block)
end
class << self
alias_method :context, :describe
alias_method :describe, :example_group
alias_method :context, :example_group

This comment has been minimized.

@myronmarston

myronmarston Jan 1, 2014

Member

Now that we have a more general mechanism for declaring example group aliases, any reason not to leverage that here rather than just alias_method?

@myronmarston

myronmarston Jan 1, 2014

Member

Now that we have a more general mechanism for declaring example group aliases, any reason not to leverage that here rather than just alias_method?

This comment has been minimized.

@myronmarston

myronmarston Jan 1, 2014

Member

There are a number of other built-in aliases I would like to see (for parity with the example aliases):

  • xdescribe (shortcut for making an example group pending, like xit)
  • xcontext (same)
  • fdescribe (shortcut for focusing on an example group, like fit)
  • fcontext (same)

I'm not sure I want to add fexample_group and xexample_group, though.

@myronmarston

myronmarston Jan 1, 2014

Member

There are a number of other built-in aliases I would like to see (for parity with the example aliases):

  • xdescribe (shortcut for making an example group pending, like xit)
  • xcontext (same)
  • fdescribe (shortcut for focusing on an example group, like fit)
  • fcontext (same)

I'm not sure I want to add fexample_group and xexample_group, though.

it "is not added to every object in the system" do
expect(Object.new).not_to respond_to(method_name)
end

This comment has been minimized.

@myronmarston

myronmarston Jan 1, 2014

Member

There's nothing here that specs the fact that main and Module do not get these methods added when expose_dsl_globally is false. Would be good to add that.

@myronmarston

myronmarston Jan 1, 2014

Member

There's nothing here that specs the fact that main and Module do not get these methods added when expose_dsl_globally is false. Would be good to add that.

This comment has been minimized.

@JonRowe

JonRowe Jan 3, 2014

Member

Also there's nothing I've seen that shows that this new code is safe against the rspec loading issue.
E.g. when you specify the aliases after rspec has loaded, it looks like they won't be exposed off main, Module unless someone toggles the expose method manually.

@JonRowe

JonRowe Jan 3, 2014

Member

Also there's nothing I've seen that shows that this new code is safe against the rspec loading issue.
E.g. when you specify the aliases after rspec has loaded, it looks like they won't be exposed off main, Module unless someone toggles the expose method manually.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jan 9, 2014

Member

@michihuber -- I'd love to get this in 3.0.0.beta2, which we'll be releasing soon. Are you able to get to get back to this in the next couple days? If not, that's fine; I'll plan to take it across the finish line so we can merge it. I just wanted to check first to make sure we don't step on each other's toes.

Member

myronmarston commented Jan 9, 2014

@michihuber -- I'd love to get this in 3.0.0.beta2, which we'll be releasing soon. Are you able to get to get back to this in the next couple days? If not, that's fine; I'll plan to take it across the finish line so we can merge it. I just wanted to check first to make sure we don't step on each other's toes.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jan 15, 2014

Member

I'm going to take a stab at getting this across the finish line. Thanks for getting this started, @michihuber!

Member

myronmarston commented Jan 15, 2014

I'm going to take a stab at getting this across the finish line. Thanks for getting this started, @michihuber!

@myronmarston myronmarston referenced this pull request Jan 22, 2014

Merged

Alias example group #1255

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jan 22, 2014

Member

Closing in favor of #1255.

Member

myronmarston commented Jan 22, 2014

Closing in favor of #1255.

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