Skip to content
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

Define derived metadata. #1496

Merged
merged 1 commit into from Apr 22, 2014
Merged

Define derived metadata. #1496

merged 1 commit into from Apr 22, 2014

Conversation

@myronmarston
Copy link
Member

myronmarston commented Apr 21, 2014

This fixes #969 and supercedes #1089.

Not ready for merge yet (or even at a point where I'd ask it to be reviewed)...I'm pushing this so I can make an rspec-rails PR that depends on this.

@JonRowe
Copy link
Member

JonRowe commented Apr 21, 2014

Ping me when you'd like a review ;)

Fixes #969 and supercedes #1089.
@myronmarston
Copy link
Member Author

myronmarston commented Apr 22, 2014

Ping...this is ready for review.

I'm not totally satisfied with the name define_derived_metadata. It's a general callback mechanism for metadata that doesn't necessarily have to be used to define derived metadata (all though that's the use case there). Not sure of a better name, though. Any thoughts?

@myronmarston myronmarston changed the title WIP: define derived metadata. Define derived metadata. Apr 22, 2014
# @private
def apply_derived_metadata_to(metadata)
@derived_metadata_blocks.each do |filter, block|
block.call(metadata) if filter.empty? || MetadataFilter.any_apply?(filter, metadata)

This comment has been minimized.

Copy link
@JonRowe

JonRowe Apr 22, 2014

Member

Why if filter.empty? here?

This comment has been minimized.

Copy link
@myronmarston

myronmarston Apr 22, 2014

Author Member

Because if you don't pass any filters, it should always apply:

RSpec.configure do |config|
  config.define_derived_metadata do |metadata|
    # this block should always run against every metadata hash
  end
end

This comment has been minimized.

Copy link
@JonRowe

JonRowe Apr 22, 2014

Member

Oh, to apply the block globally, I see.

This comment has been minimized.

Copy link
@JonRowe

JonRowe Apr 22, 2014

Member

Figured it out as you commented there ;)

@@ -84,6 +84,7 @@ def populate

populate_location_attributes
metadata.update(user_metadata)
RSpec.configuration.apply_derived_metadata_to(metadata)

This comment has been minimized.

Copy link
@JonRowe

JonRowe Apr 22, 2014

Member

Minor concern about using the singleton config here, but it's not very common to replace the config and shouldn't happen during run time so I think that's ok.

This comment has been minimized.

Copy link
@myronmarston

myronmarston Apr 22, 2014

Author Member

If there's a better way to access it I'm all ears...

@JonRowe
Copy link
Member

JonRowe commented Apr 22, 2014

I like how simple this has turned out, how about define_dynamic_metadata BTW?

@myronmarston
Copy link
Member Author

myronmarston commented Apr 22, 2014

Oh yeah, the other open question is the semantics of passing multiple filters to define_derived_metadata: should the callback only get invoked when a metadata hash matches all passed filters or any of them? any_apply is used for module inclusion filtering and example filtering. all_apply is used for hook filtering. This seems the closest to module inclusion so I aligned it with that. Any preferences?

@myronmarston
Copy link
Member Author

myronmarston commented Apr 22, 2014

how about define_dynamic_metadata BTW?

I think I like define_derived_metadata better but I have no objective reason for that preference.

@JonRowe
Copy link
Member

JonRowe commented Apr 22, 2014

I didn't know we used any_apply? for module inclusion, that's strangely inconsistent in my eyes.
I've only used example filtering via tags which are any_apply? for each --tag but all_apply? for csv (at least I think it's that way round).

I think I have a minor preference for all_apply? for parity with hooks...

@JonRowe
Copy link
Member

JonRowe commented Apr 22, 2014

I'm actually ok with define_derived_metadata but you asked for suggestions ;)

@myronmarston
Copy link
Member Author

myronmarston commented Apr 22, 2014

I didn't know we used any_apply? for module inclusion, that's strangely inconsistent in my eyes.

Well, that was the necessary semantic for rspec-rails to be able to use its old way of inclusion:

https://github.com/rspec/rspec-rails/blob/533660d10d2beeb586d99641c6b0cc280a642e05/lib/rspec/rails/example.rb#L16-L18

RSpec::Rails::ControllerExampleGroup would be included if it was in spec/controllers or if tagged with :type => :controller; it didn't require both. That makes sense to me.

I think any is a slightly better semantic because it seems more flexible...if you want the semantic to actually be all, you can still get it:

config.define_derived_metadata(:a => 1, :b => 2) do |metadata|
  if metadata.values_at(:a, :b) == [1, 2]
    # mutate metadata
  end
end

...but you can't as easily simulate the any behavior if rspec's semantic is all.

@myronmarston
Copy link
Member Author

myronmarston commented Apr 22, 2014

Good to merge?

expect(ex.metadata).to include(:description => "foo", :reverse_description => "oof")
end

it 'allows multiple configured blocks to be applied, in order of definition' do

This comment has been minimized.

Copy link
@yelled3

yelled3 Apr 22, 2014

although, I like to be able to do this, i find the syntax somewhat confusing... it seems like the 2nd line overrides the first.
how do you see this being used?

it seems to me that it would be easier to have a single derived_metadata hash, which you can just edit...

c.update_metadata { |m| m[:b1_desc] = m[:description] + " (block 1)" }
c.update_metadata { |m| m[:b2_desc] = m[:b1_desc]     + " (block 2)" }

This comment has been minimized.

Copy link
@JonRowe

JonRowe Apr 22, 2014

Member

@yelled3 this is mainly being used for rspec-rails magic to prevent mutation of the hash pre runtime.

@JonRowe
Copy link
Member

JonRowe commented Apr 22, 2014

Good to merge?

Apart from one final thought, and that's that we should now prevent the metadata hash being mutated conventionally... (Even if we just dup it where it's accessed similarly to config.formatters)

@myronmarston
Copy link
Member Author

myronmarston commented Apr 22, 2014

Apart from one final thought, and that's that we should now prevent the metadata hash being mutated conventionally... (Even if we just dup it where it's accessed similarly to config.formatters)

Hmm...I'm on the fence about that. While it's a good idea to document the fact that we recommend not mutating metadata, consider the idea that rspec-rails has been doing so for years, and until you dug into rspec/rspec-rails#825, we had never seen any problems caused by metadata mutation....so in the vast majority of cases, it works fine. I think it's more likely to confuse to users to silently ignore any mutation they've done (which would happen if we used dup) than it is that they'd run into one of the cases where mutation caused a problem.

myronmarston added a commit that referenced this pull request Apr 22, 2014
Define derived metadata.
@myronmarston myronmarston merged commit d7c2115 into master Apr 22, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@myronmarston myronmarston deleted the define-derived-metadata branch Apr 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.