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

Adds hook scope aliases `example` and `context` #1174

Merged
merged 5 commits into from Nov 15, 2013

Conversation

@fj
Copy link
Contributor

fj commented Nov 12, 2013

This allows for nicer, more sensible naming in the context of RSpec.configure blocks and elsewhere.

For example, this block sounds like it's a hook that runs once, before any example:

RSpec.configure do |config|
  config.before(:all) do       # ... confusing :(
    # do something
  end
end

But it's actually a hook that runs once before every context, which is confusing. So a better name would be:

RSpec.configure do |config|
  config.before(:context) do   # sensible!     :D
    # do something
  end
end

This change adds, in hook scopes, the ability to use :example as an alias for :each and :context as an alias for :all.

See #297 for further discussion.

elsif args.any? { |a| a.is_a?(Symbol) }
raise ArgumentError.new("You must explicitly give a scope (:each, :all, or :suite) when using symbols as metadata for a hook.")
raise ArgumentError.new("You must explicitly give a scope (:each, :all, or :suite) or scope alias (:example or :context) when using symbols as metadata for a hook.")

This comment has been minimized.

Copy link
@JonRowe

JonRowe Nov 12, 2013

Member

Given that it's technically feasible to add scope aliases, perhaps this should enumerate the keys of scope_aliases rather than hard code them?

This comment has been minimized.

Copy link
@fj

fj Nov 13, 2013

Author Contributor

It's also technically feasible to add scopes. Should we add that too?

group.example("example") {}

group.run
expect(order).to eq([1])

This comment has been minimized.

Copy link
@JonRowe

JonRowe Nov 12, 2013

Member

This is no different to the example for aliasing each, thus doesn't prove that the hook is an alias of :all how about:

order = []
group.before(:context) { order << :before_context }
group.example("example") { order << :example }
group.example("example") { order << :example }

group.run
expect(order).to eq([:before_context, :example, :example])

and similarly below

order = []
group.before(:example) { order << :before }
group.example("example") { order << :example }
group.example("example") { order << :example }

group.run
expect(order).to eq([:before, :example, :before, :example])

This comment has been minimized.

Copy link
@JonRowe

JonRowe Nov 12, 2013

Member

I'd also drop the last two specs, this proves before, and duplicate these for after showing the inverse ordering

@fj
Copy link
Contributor Author

fj commented Nov 13, 2013

I've updated the PR to accommodate @JonRowe's feedback.

:example => :each,
:context => :all,
}
end

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 13, 2013

Member

Is there any reason this is a method and not a constant? This feels like it should be a constant (since it doesn't do any calculation or call any methods to create the hash).

@myronmarston
Copy link
Member

myronmarston commented Nov 13, 2013

Two other general suggestions:

  • It would be nice to surface these new aliases to users, but I'm not sure the best way to do that. IMO, they are superior to :each and :all because of the config.before(:all) confusion. Do we deprecate :each and :all? Or do we keep them as aliases but update all the docs (both inline yard docs and cukes published on relish) to use :context and :example? If we go that route it might be nice to update the code accordingly so that :context and :example are primary and :all and :each are the aliases in the implementation.
  • You added some new private helper methods to the Hooks module. This is in-line with how the code was already structured but I'd like to get away from that. Since the module gets mixed in to a user-controlled scope (e.g. example groups) a user could inadvertantly define a helper method named known_scope? (or any of the other private helper methods) and break RSpec and have no idea why. Looking at those private helper methods, it looks like they are all stateless, acting only on the arguments given to them -- so it should be pretty trivial to move them onto a different object. See rspec/rspec-mocks#459 for a recent change like this. There may be a better refactoring that extracts a new class for some of this, but the easy fix is just changing those methods to being singleton methods on the Hooks module. @jf -- it'd be cool to fix this as part of this PR but given it was an existing issue (and not something you introduced), we can address it separately if you just want to focus on the hook scope changes.
@jf
Copy link

jf commented Nov 13, 2013

uhhhh? @myronmarston: I believe u want @fj, and not me? If so, could you pls correct that mention? thanks.

@myronmarston
Copy link
Member

myronmarston commented Nov 13, 2013

uhhhh? @myronmarston: I believe u want @fj, and not me? If so, could you pls correct that mention? thanks.

Yep, sorry about that!

@fj
Copy link
Contributor Author

fj commented Nov 13, 2013

Do we deprecate :each and :all? Or do we keep them as aliases but update all the docs (both inline yard docs and cukes published on relish) to use :context and :example? If we go that route it might be nice to update the code accordingly so that :context and :example are primary and :all and :each are the aliases in the implementation.

Deprecating :all for :context and :example for :each would be a pretty huge change that would impact large swaths of the codebase. It would probably also require something like an informative blog post/series of reminders to educate people in advance of the release.

That should be a different PR (imo).

You added some new private helper methods to the Hooks module. This is in-line with how the code was already structured but I'd like to get away from that. Since the module gets mixed in to a user-controlled scope (e.g. example groups) a user could inadvertantly define a helper method named known_scope? (or any of the other private helper methods) and break RSpec and have no idea why. Looking at those private helper methods, it looks like they are all stateless, acting only on the arguments given to them -- so it should be pretty trivial to move them onto a different object. See rspec/rspec-mocks#459 for a recent change like this. There may be a better refactoring that extracts a new class for some of this, but the easy fix is just changing those methods to being singleton methods on the Hooks module. @jf -- it'd be cool to fix this as part of this PR but given it was an existing issue (and not something you introduced), we can address it separately if you just want to focus on the hook scope changes.

This is a good suggestion, and I agree that we shouldn't be namespace-squatting like that. I will make this change later and update the PR accordingly.

@myronmarston
Copy link
Member

myronmarston commented Nov 13, 2013

Deprecating :all for :context and :example for :each would be a pretty huge change that would impact large swaths of the codebase. It would probably also require something like an informative blog post/series of reminders to educate people in advance of the release.

I agree. I think the ROI on deprecating (and eventual removal) of :each and :all is pretty small. At this point, I'm leaning towards not deprecating them at all, and simply updating docs and RSpec's internal code to make the new names primary.

@JonRowe
Copy link
Member

JonRowe commented Nov 14, 2013

Leaving :each and :all as is for 3.x but updating the documentation to use the new ones gets my vote.

@JonRowe
Copy link
Member

JonRowe commented Nov 14, 2013

Also this looks ready to go in my eyes, we can do the docs etc separately.

@fj
Copy link
Contributor Author

fj commented Nov 14, 2013

I still need to do @myronmarston's second suggestion. The PR hasn't been
updated for that just yet.
On Nov 14, 2013 2:55 AM, "Jon Rowe" notifications@github.com wrote:

Also this looks ready to go in my eyes, we can do the docs etc separately.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1174#issuecomment-28465441
.

@fj
Copy link
Contributor Author

fj commented Nov 14, 2013

BTW, since we'll have to lift SCOPES and SCOPE_ALIASES so that they're accessible to the singleton methods, how do you prefer that those be realized?

I can't find any existing examples of this in the RSpec codebase and there's lots of ways to skin that cat, so I figured I'd ask first about what your preference is.

@myronmarston
Copy link
Member

myronmarston commented Nov 14, 2013

BTW, since we'll have to lift SCOPES and SCOPE_ALIASES so that they're accessible to the singleton methods, how do you prefer that those be realized?

Not sure I follow. Constants are accessible to class methods and instance methods:

class Foo
  CONST = 3

  def can_access_const
    CONST
  end

  def self.can_access_const
    CONST
  end
end
@fj
Copy link
Contributor Author

fj commented Nov 14, 2013

@myronmarston Sorry, it was late when I wrote that and I was tired, and I'm not sure what I meant either. You are correct, of course.

@myronmarston
Copy link
Member

myronmarston commented Nov 14, 2013

No worries :).

myronmarston added a commit that referenced this pull request Nov 15, 2013
Adds hook scope aliases `example` and `context`
@myronmarston myronmarston merged commit 9edaf55 into rspec:master Nov 15, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
yujinakayama added a commit to yujinakayama/rspec-core that referenced this pull request Mar 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.

None yet

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