Skip to content
This repository has been archived by the owner on Nov 30, 2024. It is now read-only.

Support user-supplied metadata for #its #1080

Closed
wants to merge 4 commits into from

Conversation

AlphaHydrae
Copy link

I'm working on a test analysis tool which relies heavily on user-supplied metadata such as this:

it "should work", :meta => :data do
  # ...
end

There's currently no way to add metadata to an example defined with its aside from wrapping it in an example group:

subject{ "foo" }
describe "group for metadata", :meta => :data do
  its(:to_s){ should eq("foo") }
end

This is very verbose when you have many its examples. With the changes in this commit, its forwards additional arguments to its internal describe call, allowing this:

subject{ "foo" }
its(:to_s, :meta => :data){ should eq("foo") }

@coveralls
Copy link

Coverage Status

Coverage increased (+14.52%) when pulling 2a79744 on lotaris:its-metadata into 9362e65 on rspec:master.

@JonRowe
Copy link
Member

JonRowe commented Sep 19, 2013

Thanks for this, just as an FYI if we do merge this it will be extracted to the separate its gem when we get around to that ;)

Also this will need a change log entry...

@@ -52,6 +52,13 @@ Feature: User-defined metadata
example.metadata[:bazz].should eq(33)
example.metadata.should_not include(:foo)
end

describe "has an #its example with metadata" do
subject{ "foo" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finicky I know but subject { "foo" } please.

@AlphaHydrae
Copy link
Author

You're welcome. I fixed the spacing. Hadn't noticed that convention. I'll add a changelog entry as well.

@coveralls
Copy link

Coverage Status

Coverage increased (+14.52%) when pulling 78d9bab on lotaris:its-metadata into 9362e65 on rspec:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+14.52%) when pulling dd817e7 on lotaris:its-metadata into 9362e65 on rspec:master.

@@ -43,6 +43,7 @@ Enhancements
(Thomas Stratmann, Myron Marston).
* Document the configuration options and default values in the `spec_helper.rb`
file that is generated by RSpec (Parker Selbert).
* `its` examples can have user-supplied metadata.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change logs entries usually indicate their author ;)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. I am obviously tired today. Sorry about the commit fest.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record we don't mind squashing and force pushing on PR's but I'm also ok with the 4 commits, I've written worse PR's!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know! I'll leave it like this then, so I can put it in my hall of shame. Thanks.

@myronmarston
Copy link
Member

@AlphaHydrae - thanks for the well written PR! However, my instinct is to close this w/o merging it for a few reasons:

  • As we've previously stated, we're removing its from rspec-core in the next release. I don't want to add to a feature that's going away from this gem before we do another release.
  • Users have asked for its to support arguments on multiple occasions (e.g. its(:method_name, :arg_to_method)). While we've resisted adding that to rspec-core's its, once extracted from rspec-core and moved into its own gem, I had always assumed the new maintainers would revisit and potentially add that feature. Unfortunately, it's incompatible with what you've added here: either additional arguments can be forwarded to the method as args or forwarded to describe as args, but not both. So, I'd like to leave it up to the new maintainers of the rspec-its gem to decide which direction they want to go (hopefully getting community feedback) and not eliminate that possibility for them by merging this now.
  • IMO, it's non-obvious that the additional args are metadata. I'd be more likely to think they are arguments to the method.
  • its isn't meant to be as fully flexible (e.g. with metadata and everything) as normal examples. If you need that flexibility, my recommending is to define an example normally.

Let me know if you have any questions/concerns about this, and thanks again for the good PR. We hope to see more contributions from you in the future.

@AlphaHydrae
Copy link
Author

@myronmarston Ok, I see your point. Maybe I'll do another PR in the new gem when the time comes.

I didn't know other users wanted to add arguments. That reminds me of the arguments to it_behaves_like, although it seems less useful for its. But I guess it can make sense. We'll see.

@myronmarston
Copy link
Member

I didn't know other users wanted to add arguments.

See #553 and #306.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants