Adds hook scope aliases `example` and `context` by fj · Pull Request #1174 · rspec/rspec-core

fj · 2013-11-12T12:08:32Z

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.

JonRowe · 2013-11-12T22:22:44Z

lib/rspec/core/hooks.rb

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

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

fj · 2013-11-13T14:26:53Z

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

myronmarston · 2013-11-13T15:59:39Z

lib/rspec/core/hooks.rb

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 · 2013-11-13T16:11:20Z

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 Squat on less of the user's method namespace. 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 · 2013-11-13T16:23:16Z

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

myronmarston · 2013-11-13T16:24:20Z

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

Yep, sorry about that!

fj · 2013-11-13T22:02:46Z

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 · 2013-11-13T22:17:10Z

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 · 2013-11-14T07:55:22Z

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

JonRowe · 2013-11-14T07:55:43Z

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

fj · 2013-11-14T11:22:11Z

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 · 2013-11-14T13:10:45Z

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 · 2013-11-14T15:50:32Z

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 · 2013-11-14T15:57:57Z

@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 · 2013-11-14T16:05:00Z

No worries :).

Adds hook scope aliases `example` and `context`

Adds hook scope aliases example and context

9bf8ae3

fj mentioned this pull request Nov 12, 2013

Add hook scope aliases :example and :group #297

Closed

JonRowe reviewed Nov 12, 2013
View reviewed changes

fj added 2 commits November 13, 2013 09:24

Automatically generates scope/alias names in error messages

15b1b37

Reworks example-group scope-alias specs to be less ambiguous

b0aad22

myronmarston reviewed Nov 13, 2013
View reviewed changes

Refactors scope_aliases method ⇒ SCOPE_ALIASES constant hash

e8202dc

Lifts new private instance methods to singleton methods

9612f11

myronmarston added a commit that referenced this pull request Nov 15, 2013

Merge pull request #1174 from fj/topic/scope-aliases

9edaf55

Adds hook scope aliases `example` and `context`

myronmarston merged commit 9edaf55 into rspec:master Nov 15, 2013

myronmarston mentioned this pull request Nov 15, 2013

Update docs to reference new hook scope names :context and :example #1184

Closed

This was referenced Mar 22, 2014

Consider supporting conversion of hook scope names yujinakayama/transpec#53

Closed

Add missing changelog entry for the addition of hook scope aliases #1426

Merged

Uh oh!

Conversation

fj commented Nov 12, 2013

Uh oh!

JonRowe Nov 12, 2013

Choose a reason for hiding this comment

Uh oh!

fj Nov 13, 2013

Choose a reason for hiding this comment

Uh oh!

fj commented Nov 13, 2013

Uh oh!

myronmarston Nov 13, 2013

Choose a reason for hiding this comment

Uh oh!

myronmarston commented Nov 13, 2013

Uh oh!

jf commented Nov 13, 2013

Uh oh!

myronmarston commented Nov 13, 2013

Uh oh!

fj commented Nov 13, 2013

Uh oh!

myronmarston commented Nov 13, 2013

Uh oh!

JonRowe commented Nov 14, 2013

Uh oh!

JonRowe commented Nov 14, 2013

Uh oh!

fj commented Nov 14, 2013

Uh oh!

fj commented Nov 14, 2013

Uh oh!

myronmarston commented Nov 14, 2013

Uh oh!

fj commented Nov 14, 2013

Uh oh!

myronmarston commented Nov 14, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants