Aliasing API for #describe #870

Closed
wants to merge 15 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

michihuber commented Apr 11, 2013

This adds the ability to configure aliases for #describe with default metadata, as requested in #493.

  • Renames ExampleGroup.describe to .example_group (and aliases it back to .describe).
  • Adds Configuration#alias_example_group_to, which accepts default metadata and a :toplevel_alias option.
  • Adds --toplevel-off flag, which avoids extending the main object with the DSL methods (i.e. #describe and defined aliases).

@myronmarston myronmarston commented on an outdated diff Apr 11, 2013

lib/rspec/core/configuration.rb
@@ -604,6 +604,13 @@ def alias_example_to(new_name, *args)
RSpec::Core::ExampleGroup.alias_example_to(new_name, extra_options)
end
+ def alias_example_group_to(new_name, *metadata_and_opts)
+ top_level_method = !!metadata_and_opts.delete(:toplevel_alias)
@myronmarston

myronmarston Apr 11, 2013

Owner

This has the side effect of modifying the passed in hash. Consider a case like this:

RSpec.configure do |c|
  focused_options = { focus: true, toplevel_alias: true }
  c.alias_example_group_to :focus, focused_options
  c.alias_example_group_to :focused, focused_options
end

The 2nd one will not work properly because the first one modifies the passed in options.

I think it's better to dup the hash before deleting from it.

@myronmarston myronmarston commented on an outdated diff Apr 11, 2013

lib/rspec/core/configuration.rb
@@ -604,6 +604,13 @@ def alias_example_to(new_name, *args)
RSpec::Core::ExampleGroup.alias_example_to(new_name, extra_options)
end
+ def alias_example_group_to(new_name, *metadata_and_opts)
+ top_level_method = !!metadata_and_opts.delete(:toplevel_alias)
+ extra_options = build_metadata_hash_from(metadata_and_opts)
@myronmarston

myronmarston Apr 11, 2013

Owner

I think this line and the prior one are out-of-order: before deleting the toplevel_alias key from the hash, you want the passed-in-args normalized to a hash, right? A specific case where I think this logic will fail is:

RSpec.configure do |c|
  c.alias_example_group_to :focus, :focus, toplevel_alias: true
end

...i.e. where symbols and a full-on hash are mixed. When this is passed, metadata_and_opts will be passed in as [ :focus, { toplevel_alias: true } ], right? So the delete above will fail anyway.

Actually, if you re-order these two lines, I think it'll solve the modifying-the-passed-in-hash-as-a-side-effect-issue.

Regardless, it'd be good to have specs to cover both of these cases.

@myronmarston myronmarston commented on an outdated diff Apr 11, 2013

lib/rspec/core/dsl.rb
@@ -2,7 +2,18 @@ module RSpec
module Core
# Adds the `describe` method to the top-level namespace.
module DSL
- # Generates a subclass of {ExampleGroup}
+ # Generates a method that passes on the message to
+ # generate a subclass of {ExampleGroup}
+ #
+
+ def self.register_example_group_alias(name)
+ define_method(name) do |*args, &example_group_block|
+ RSpec::Core::ExampleGroup.public_send(name, *args, &example_group_block).register
+ end
@myronmarston

myronmarston Apr 11, 2013

Owner

This syntax causes 1.8.6 to blow up. (Its parser can't handle blocks accepting blocks).

We're planning on dropping 1.8.6 support in 3.0, and starting on 3.0 as soon as 2.14 ships, so we can just hold off on merging this until we get started on 3.0. If you want to get this in 2.14 you'll have to change stuff so it's 1.8.6 compatible, but that would be a bunch of work that we would undo as soon as 3.0 starts, because we don't want to keep the 1.8.6 hacks in 3.0.

Thoughts?

@myronmarston myronmarston commented on an outdated diff Apr 11, 2013

lib/rspec/core/dsl.rb
@@ -2,7 +2,18 @@ module RSpec
module Core
# Adds the `describe` method to the top-level namespace.
module DSL
- # Generates a subclass of {ExampleGroup}
+ # Generates a method that passes on the message to
+ # generate a subclass of {ExampleGroup}
+ #
+
+ def self.register_example_group_alias(name)
@myronmarston

myronmarston Apr 11, 2013

Owner

Any reason for the blank line between the comment and here? I'm not sure if YARD picks up on the comment applying to this method if there's a blank line in between...

@myronmarston myronmarston commented on an outdated diff Apr 11, 2013

lib/rspec/core/example_group.rb
@@ -139,6 +139,19 @@ def #{new_name}(name, *args, &customization_block)
def alias_it_behaves_like_to name, *args, &block
(class << self; self; end).define_nested_shared_group_method name, *args, &block
end
+
+ # Works like `alias_method :name, :example_group` with the added benefit of
+ # assigning default metadata to the generated example group.
+ #
+ # @note Use with caution. This extends the language used in your
+ # specs, but does not add any additional documentation.
+ def alias_example_group_to(name, metadata={})
+ define_singleton_method(name) do |*args, &block|
@myronmarston

myronmarston Apr 11, 2013

Owner

This is another place that'll not parse on 1.8.6, and leads me to suggest waiting until 3.0 to merge this.

In addition, define_singleton_method isn't available on 1.8.7 (which we do plan to support in 3.0).

@myronmarston myronmarston commented on an outdated diff Apr 11, 2013

lib/rspec/core/dsl.rb
@@ -2,7 +2,18 @@ module RSpec
module Core
# Adds the `describe` method to the top-level namespace.
module DSL
- # Generates a subclass of {ExampleGroup}
+ # Generates a method that passes on the message to
+ # generate a subclass of {ExampleGroup}
+ #
+
+ def self.register_example_group_alias(name)
+ define_method(name) do |*args, &example_group_block|
+ RSpec::Core::ExampleGroup.public_send(name, *args, &example_group_block).register
@myronmarston

myronmarston Apr 11, 2013

Owner

I don't think public_send is available on 1.8.7, which we plan to support in 3.0.

@myronmarston myronmarston commented on an outdated diff Apr 11, 2013

lib/rspec/core/example_group.rb
@@ -139,6 +139,19 @@ def #{new_name}(name, *args, &customization_block)
def alias_it_behaves_like_to name, *args, &block
(class << self; self; end).define_nested_shared_group_method name, *args, &block
end
+
+ # Works like `alias_method :name, :example_group` with the added benefit of
+ # assigning default metadata to the generated example group.
+ #
+ # @note Use with caution. This extends the language used in your
+ # specs, but does not add any additional documentation.
+ def alias_example_group_to(name, metadata={})
+ define_singleton_method(name) do |*args, &block|
+ combined_metadata = metadata.dup
+ combined_metadata.merge!(args.delete_at(-1)) if args.last.is_a? Hash
@myronmarston

myronmarston Apr 11, 2013

Owner
  • I think args.pop is a simpler way of returning (and removing) the last item, rather than args.delete_at(-1). Thoughts?
  • I don't think this will work properly with the "symbols-as-metadata-keys-with-true-values" feature we support. I think you're going to have to use build_metadata_hash_from to normalize it to a hash first.

...actually, I'm not sure about the last bullet point, now that I see that you're passing args and combined_metadata through...it'd be good to have a spec for the case at least.

@myronmarston myronmarston commented on an outdated diff Apr 11, 2013

lib/rspec/core/example_group.rb
@@ -139,6 +139,19 @@ def #{new_name}(name, *args, &customization_block)
def alias_it_behaves_like_to name, *args, &block
(class << self; self; end).define_nested_shared_group_method name, *args, &block
end
+
+ # Works like `alias_method :name, :example_group` with the added benefit of
+ # assigning default metadata to the generated example group.
+ #
+ # @note Use with caution. This extends the language used in your
+ # specs, but does not add any additional documentation.
+ def alias_example_group_to(name, metadata={})
+ define_singleton_method(name) do |*args, &block|
+ combined_metadata = metadata.dup
+ combined_metadata.merge!(args.delete_at(-1)) if args.last.is_a? Hash
+ describe(*args, combined_metadata, &block)
@myronmarston

myronmarston Apr 11, 2013

Owner

It seems like example_group is the primary method name now (with describe and context being aliases for it), so maybe this should use example_group?

@myronmarston myronmarston commented on an outdated diff Apr 11, 2013

lib/rspec/core/option_parser.rb
@@ -95,6 +95,10 @@ def parser(options)
options[:drb_port] = o.to_i
end
+ parser.on('--toplevel-off', "Don't include DSL methods into main Object") do |o|
@myronmarston

myronmarston Apr 11, 2013

Owner

How about into the main Object rather than into main Object?

@myronmarston

myronmarston Apr 11, 2013

Owner

Also, --toplevel-off is a bit non-standard compared to our other CLI options. Elsewhere we use --no-<opt> and --<opt> (e.g. --no-color and --color).

What do you think about calling this --no-toplevel-dsl and --toplevel-dsl?

@myronmarston

myronmarston Apr 11, 2013

Owner

BTW, I was thinking that it would be nice if this was an option from RSpec.configure, too...but that might be tricky to get right (given that people may have a top-level describe before RSpec.configure) -- and it seems complicated to figure out a way to get that to work. I'm not sure it's worth the effort, but it would be nice!

@myronmarston myronmarston commented on an outdated diff Apr 11, 2013

lib/rspec/core/runner.rb
@@ -47,6 +48,12 @@ def self.trap_interrupt
end
end
+ def self.set_up_dsl
+ # sends extend to the main-Object
+ TOPLEVEL_BINDING.eval('self').send(:extend, RSpec::Core::DSL)
@myronmarston

myronmarston Apr 11, 2013

Owner

I'd prefer not to add new string evals (see #862), and this is a bit of an obtuse way to get main. What do you think about doing this instead?

module RSpec
  module Core
    class Runner
      class << self
        attr_accessor :main
      end
    end
  end
end

RSpec::Core::Runner.main = self

...then you can just get main from the main attribute.

@myronmarston myronmarston commented on the diff Apr 11, 2013

spec/rspec/core/configuration_spec.rb
+ describe "#alias_example_group_to" do
+ after do
+ RSpec::Core::ExampleGroup.module_eval do
+ class << self
+ undef :my_group_method if method_defined? :my_group_method
+ end
+ end
+ end
+
+ it_behaves_like "metadata hash builder" do
+ def metadata_hash(*args)
+ config.alias_example_group_to :my_group_method, *args
+ group = ExampleGroup.my_group_method("a group")
+ group.metadata
+ end
+ end
@myronmarston

myronmarston Apr 11, 2013

Owner

I'm glad to see you re-used my shared example group here 👍

@myronmarston myronmarston commented on the diff Apr 11, 2013

spec/rspec/core/configuration_spec.rb
@@ -1231,6 +1231,30 @@ def host.foobar; end
end
end
+ describe "#alias_example_group_to" do
+ after do
+ RSpec::Core::ExampleGroup.module_eval do
+ class << self
+ undef :my_group_method if method_defined? :my_group_method
+ end
+ end
+ end
@myronmarston

myronmarston Apr 11, 2013

Owner

Nice way of cleaning up!

@myronmarston myronmarston commented on an outdated diff Apr 11, 2013

features/example_groups/aliasing.feature
+ end
+ end
+ end
+ """
+ When I run `rspec nested_example_group_aliases_spec.rb --tag detailed -fdoc`
+ Then the output should contain:
+ """
+ a thing
+ something less important
+ """
+
+ Scenario: custom example group alias at the top-level
+ Given a file named "top_level_example_group_aliases_spec.rb" with:
+ """ruby
+ RSpec.configure do |c|
+ c.alias_example_group_to :detail, :toplevel_alias
@myronmarston

myronmarston Apr 11, 2013

Owner

Seeing this in context make me think that the _alias suffix for the :toplevel_alias option is unnecessary, given the method already has alias in it.

Also, I brought this up on the original issue request, but it bothers me a little bit that this option is treated specially and is not added to the metadata hash....that seems inconsistent, and would certainly be surprising to someone who didn't know there was a :toplevel_alias option, and was simply trying to use it as a metadata key. I'm not sure what to do about this, though.

@myronmarston myronmarston commented on the diff Apr 11, 2013

features/example_groups/aliasing.feature
+ it "can do things" do
+ end
+ end
+
+ detail "something less important" do
+ it "can do an unimportant thing" do
+ end
+ end
+ end
+ """
+ When I run `rspec nested_example_group_aliases_spec.rb --tag detailed -fdoc`
+ Then the output should contain:
+ """
+ a thing
+ something less important
+ """
@myronmarston

myronmarston Apr 11, 2013

Owner

I think it's also important to have a step that says:

And the output should not contain "in broad strokes"

As this scenario stands, it could feasible produce the output you've specified here while not actually skipping the "broad strokes" example -- i.e. if the randomization feature was being used.

@myronmarston myronmarston commented on an outdated diff Apr 11, 2013

features/example_groups/aliasing.feature
+ """
+ a thing
+ works
+ """
+
+ Scenario: Turn off toplevel methods
+ Given a file named "top_level_example_group_aliases_spec.rb" with:
+ """ruby
+ describe "is not available" do
+ end
+ """
+ When I run `rspec --toplevel-off top_level_example_group_aliases_spec.rb -fdoc`
+ Then the output should contain:
+ """
+ undefined method `describe'
+ """
@myronmarston

myronmarston Apr 11, 2013

Owner

I believe this option will also disable other top-level DSL methods like shared_examples_for and shared_context, right? Is there a reasonable way to demonstrate/document that here?

Owner

myronmarston commented Apr 11, 2013

@michihuber -- fantastic work. This is very well written! A few additional thoughts:

  • When the top-level DSL methods are turned off, how are users supposed to declare example groups? That's not documented anywhere. I know (from working on RSpec) that users can use RSpec::Core::ExampleGroup.describe -- but that's not obvious to end users, is not documented anywhere, is kinda an ugly API and isn't really meant to be used publicly. What about always extending the DSL module onto RSpec? Then when folks turn off the top-level DSL, they could still do:
RSpec.shared_examples_for "some behavior" do
end

RSpec.describe MyClass do
end
  • This combined with rspec/rspec-mocks#266 gets us within range of having an RSpec that doesn't monkey patch anything...which would be pretty awesome. My initial thought was that maybe the --toplevel-off (or whatever we call it) option should default to true in 3.0, but now I'm thinking that it's a big change, and we should simply advertise that in rspec 3 you'll be able to use it w/o any monkey patching, but then in 4.0 we can actually make it the default if the community is behind it by that point. This sort of migration path will be a lot friendlier to newbies that are using old resources to try out rspec. Thoughts from others on this?
Owner

myronmarston commented Apr 11, 2013

BTW, in some of my comments above I mentioned "the original issue"...I actually meant #790 (the one I knew of and had participated in), not #493 -- I didn't even realize that's been a long-standing open issue.

Contributor

michihuber commented Apr 11, 2013

Thanks for your detailed comments, @myronmarston, much appreciated! I totally missed #790, but reread it now. You weren't sure about toplevel support, what do you think about it now? I think the overhead is reasonable, but it would introduce a "reserved" metadata key, as you pointed out above. What about a different method? #toplevel_alias_example_group_to?

I wanted to get input about where the DSL should be included in first, I'll add it to RSpec.

I'll fix the stuff you commented on (and the failing specs) in the next few days.

Contributor

michihuber commented Apr 11, 2013

From my side, a 3.0 release without 1.8.6 is fine, but I'd make the necessary changes of course if somebody needs it earlier.

Owner

myronmarston commented Apr 11, 2013

You weren't sure about toplevel support, what do you think about it now? I think the overhead is reasonable, but it would introduce a "reserved" metadata key, as you pointed out above.

I like the feature a lot now, because it gives us the possibility of having a zero-monkey-patching RSpec. That's really cool.

As for the reserved metadata key...I'm on the fence about it. We already have a couple of those (e.g. pending). It can lead to surprises though.

What about a different method? #toplevel_alias_example_group_to?

Maybe...problem is, that suggests that the alias is only at the top level. More thought is needed, I think.

/cc @JonRowe @alindeman @samphippen @soulcutter

Owner

JonRowe commented Apr 13, 2013

I also like the idea of having zero monkey patching :) but I think we'd definitely want an easy way to access the DSL if top level was turned off.. I'm not sure I like polluting RSpec directly though... maybe we could go with something like RSpec::DSL.describe?

Also, crazy thought but could we disable mixing in describe to the top level, but then have the runner eval _spec files within the context of that DSL? Thus removing monkey patching but also allowing a legacy style?

Contributor

michihuber commented Apr 14, 2013

I've addressed most of @myronmarston's comments.

Some open questions:

  • From where should the DSL be proxied? RSpec? RSpec::DSL?
  • How should aliases to the top-level be distinguished from nested aliases? Two distinct methods avoid the reserved keyword problem, so I think that makes sense, but a fitting name is needed.
  • Regarding a toplevel_dsl config option: that was actually the approach I took first, but I guess this could lead to lots of surprises. I'll take another stab at it and try to find a good way to guide through warnings etc.
  • I also really like @JonRowe's idea (evaluating the spec files in the DSL's scope). I took a quick stab today and it looks very promising, I'll try to work out the remaining kinks in the next view days (most of them have to do with running specs with line numbers).
Owner

myronmarston commented Apr 14, 2013

I'm not sure I like polluting RSpec directly though... maybe we could go with something like RSpec::DSL.describe?

What problem do you see with "polluting" RSpec? I don't actually see it as polluting RSpec at all; I see it as exposing the API in the form we want it. I don't think we need the ceremony of Nested::Constants, and I can't think of a single problem this will cause. For end users, I think RSpec.describe feels very simple and straightforward in a way that RSpec::DSL.describe doesn't.

Also, crazy thought but could we disable mixing in describe to the top level, but then have the runner eval _spec files within the context of that DSL? Thus removing monkey patching but also allowing a legacy style?

This is a very interesting idea that has never occurred to me! (That's the great thing about bringing new folks on board; they bring new ideas...) However, I'm concerned about the repercussions of such a change:

  • Sometimes folks rely on self at the top level of their spec files being main; for example here's one place where I did that. If we made this change, such things would break.
  • Other times, folks define levels of the top level, expecting them to be added as helper methods to all objects. This works when files are loaded normally, but would not work if we eval files in a different context.
  • In general, I think it's best for RSpec to do as much "normal" ruby stuff, and as little surprising, non-standard things as possible. Changing what context spec files are eval'd in would be a very surprising change, I think. I don't think the benefit is large enough to warrant making the change. There are likely to be unforeseen negative repercussions, I think
Owner

JonRowe commented Apr 14, 2013

What problem do you see with "polluting" RSpec? I don't actually see it as polluting RSpec at all; I see it as exposing the API in the form we want it. I don't think we need the ceremony of Nested::Constants, and I can't think of a single problem this will cause. For end users, I think RSpec.describe feels very simple and straightforward in a way that RSpec::DSL.describe doesn't.

Fair enough, I just edge away from everything being on the top level, but I guess a .describe is pretty core functionality... I accept your reasoning 👍

Sometimes folks rely on self at the top level of their spec files being main; for example here's one place where I did that. If we made this change, such things would break.

Hmm. We could make main available to people to work around this, altho I feel it's very edge case?

Other times, folks define levels of the top level, expecting them to be added as helper methods to all objects. This works when files are loaded normally, but would not work if we eval files in a different context.

They should continue to work as is inside RSpec, as everything would be nested... Support files would also continue to be defined from main.

In general, I think it's best for RSpec to do as much "normal" ruby stuff, and as little surprising, non-standard things as possible.

Agreed.

Changing what context spec files are eval'd in would be a very surprising change, I think. I don't think the benefit is large enough to warrant making the change. There are likely to be unforeseen negative repercussions, I think

Like I said it was a crazy thought, I was mostly concerned with how we could remove monkey patching whilst preserving existing functionality for our users.

Owner

JonRowe commented Dec 7, 2013

@michihuber want to try rebasing this now the monkey patching PR is merged?

Owner

myronmarston commented Dec 7, 2013

BTW, regarding whether or not this config option should add top-level describe alias: I think that should depend on the setting @JonRowe added in #1036. That'll be cleaner than forcing the user to pass a particular hash key to specify that. We should probably also make context top-level (or not) based on that new config option.

Contributor

michihuber commented Dec 7, 2013

Sure, I’ll update this to use @JonRowe’s new setting in the upcoming week.

On Saturday 7 December 2013 at 17:42, Myron Marston wrote:

BTW, regarding whether or not this config option should add top-level describe alias: I think that should depend on the setting @JonRowe (https://github.com/JonRowe) added in #1036 (#1036). That'll be cleaner than forcing the user to pass a particular hash key to specify that. We should probably also make context top-level (or not) based on that new config option.


Reply to this email directly or view it on GitHub (#870 (comment)).

Contributor

michihuber commented Dec 15, 2013

@myronmarston, I think it might be useful to allow making describe available from the top level without also adding all other example_group aliases (context or custom ones). I.e. @JonRowe's expose_globally should only control the scope of the API, not the content.

What about two different config methods instead of a reserved hash key? I proposed alias_example_group_to and toplevel_alias_example_group_to, but you were suggesting that the latter might imply that the alias would only exist at the top level.

Maybe finding a new method name might be worthwhile and less confusing than expose_globally changing scope and content.

Owner

JonRowe commented Dec 15, 2013

@michihuber you'll note there will need to be some rejigging to make my code work with custom aliases, so it'd be nice to refactor it to add the default aliases but also any custom ones in the same mechanism

Owner

myronmarston commented Dec 16, 2013

@myronmarston, I think it might be useful to allow making describe available from the top level without also adding all other example_group aliases (context or custom ones). I.e. @JonRowe's expose_globally should only control the scope of the API, not the content.

Why is that useful? It's unclear to me. FWIW, context used to be available at the top-level, but it was removed because at the time the implementation added context to every object, and led to some conflicts. Now we have a much better, safer way of exposing that top-level DSL that adds it only to main and modules, and does not add it to every object. If a user doesn't want to use a particular alias from the top-level...then they should simply not call that method. I don't think it's worth the complexity of adding additional config options, now that we have the expose_dsl_globally for toggling it on and off at the top level.

Contributor

michihuber commented Dec 16, 2013

Sure, that makes sense.
I didn’t realise before that you wanted to make context an alias for describe by default (as I also didn’t know how that difference came about).

On Monday 16 December 2013 at 01:10, Myron Marston wrote:

@myronmarston (https://github.com/myronmarston), I think it might be useful to allow making describe available from the top level without also adding all other example_group aliases (context or custom ones). I.e. @JonRowe (https://github.com/JonRowe)'s expose_globally should only control the scope of the API, not the content.

Why is that useful? It's unclear to me. FWIW, context used to be available at the top-level, but it was removed because at the time the implementation added context to every object, and led to some conflicts. Now we have a much better, safer way of exposing that top-level DSL that adds it only to main and modules, and does not add it to every object. If a user doesn't want to use a particular alias from the top-level...then they should simply not call that method. I don't think it's worth the complexity of adding additional config options, now that we have the expose_dsl_globally for toggling it on and off at the top level.


Reply to this email directly or view it on GitHub (#870 (comment)).

Owner

myronmarston commented Jan 9, 2014

Close in favor of #1236.

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