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

Add hook scope aliases `:example` and `:group` #297

Closed
wants to merge 1 commit into from

Conversation

@fj
Copy link
Contributor

@fj fj commented Feb 2, 2011

As per a discussion on the mailing list, Evgeniy Dolzhenko suggested that if we were going to have aliases, it would be optimal to have a change that introduced the aliases of :example, and :group, to parallel the existing :suite alias.

Here is a change that does this.

@dchelimsky
Copy link
Member

@dchelimsky dchelimsky commented Feb 5, 2011

FYI - I need to let this stew a bit before doing it. I'm personally a big fan, but I don't want to create confusion for many to ease confusion of few. Might be something to include in a 3.0 release, which I don't want to do for a while (probably not before 6 months since the 2.0 release). So I'm leaving this open for the moment.

@fj
Copy link
Contributor Author

@fj fj commented Feb 5, 2011

Sure thing; thanks, David.

@Peeja
Copy link
Contributor

@Peeja Peeja commented Dec 26, 2012

Triage fairy here.

Do these years-old issues that are in the 3.0 milestone need to be bumped and re-discussed, or are they in 3.0 so people don't have to think about them for now and I should leave them alone?

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Dec 26, 2012

@Peeja -- thanks for helping triage issues!

I generally tag things as "3.0" so that we keep the ticket open (since it's a valid issue) but make it obvious we're not planning on addressing it before 3.0.

@EarthCitizen
Copy link

@EarthCitizen EarthCitizen commented Feb 15, 2013

:example and :group seem a lot clearer to me than :all and :each. I often have to explain to people the difference between them. :example and :group would require less explanation I think. More intuitive.

@JonRowe
Copy link
Member

@JonRowe JonRowe commented Mar 16, 2013

Personally... I feel it's less intuitive, because we don't make clear what an example or a group is, (I know those are the names internally but this isn't test unit we don't expose those to people).

I'd support these aliases though, some folk will like them, some won't. I'd prefer :spec and :context over :example and :group... maybe...

@christhekeele
Copy link

@christhekeele christhekeele commented Jun 4, 2013

I'm actually pretty excited for this.

@penelopezone
Copy link
Member

@penelopezone penelopezone commented Jun 4, 2013

perhaps before(:each_spec) and before(:this_context)?

@penelopezone
Copy link
Member

@penelopezone penelopezone commented Jun 11, 2013

@JonRowe @soulcutter @myronmarston @alindeman what do you think about my names? I think it'd be good to close this one out sooner rather than later.

@soulcutter
Copy link
Member

@soulcutter soulcutter commented Jun 11, 2013

I think I prefer before(:example) and before(:context) (a mix of the original suggestion and @JonRowe 's). I'm not sure it's worth fiddling with too much, though, so the original suggestion is just fine by me.

I will say don't like :each_spec or :this_context since they are wordier and just don't read as well with the underscores.

@penelopezone
Copy link
Member

@penelopezone penelopezone commented Jun 11, 2013

I think :example and :context is also good. @fj would you mind updating this to use those symbols?

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Jun 11, 2013

I think it's worth while to change the aliases if a majority of the community finds them to be more clear. However, there hasn't yet been a consensus: between the original mailing list thread and this discussion, there have been something like 8 different proposals for new aliases. I don't think there's enough consensus on this yet.

Personally, if we're going to change it, my vote would be for :example and :group. But as I said, I don't think we have a consensus yet.

@fj
Copy link
Contributor Author

@fj fj commented Jun 11, 2013

FYI, I'm happy to update the PR and will take ownership whenever you feel consensus has been reached.

@myronmarston How would you propose we discover what the consensus is? Straw poll on the mailing list?

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Jun 11, 2013

@myronmarston How would you propose we discover what the consensus is? Straw poll on the mailing list?

I think asking folks to comment here would be a good way to go about it. We can spread the word through twitter and the mailing list.

@fj
Copy link
Contributor Author

@fj fj commented Jun 11, 2013

OK, consider this comment the poll then.

There is a proposal to add sensible aliases for :each and :all to mirror the :suite alias. Please cast a vote for what you think aliases for :each and :all should be called:

  • :example and :group, respectively
  • :example and :context, respectively
  • :spec and :context, respectively
  • Something else (please specify)
  • I don't think we need aliases
@fj
Copy link
Contributor Author

@fj fj commented Jun 11, 2013

+1 for :example and :context.

@christhekeele
Copy link

@christhekeele christhekeele commented Jun 11, 2013

+1 for :example and :group.

@batasrki
Copy link

@batasrki batasrki commented Jun 11, 2013

If we need to add aliases, I'll +1 for :example and :group

@vothane
Copy link

@vothane vothane commented Jun 11, 2013

+1 for :example and :context.

@ywen
Copy link

@ywen ywen commented Jun 11, 2013

I might be biased but after so many years with each and all, I find example and group to be confusing. Seriously, if we want to do something in this area, why not just remove support for all?

@ywen
Copy link

@ywen ywen commented Jun 11, 2013

Ah didn't read about the vote comment. I vote for we don't need aliases. My reasoning is more aliases will just lead to more confusion.

@sferik
Copy link

@sferik sferik commented Jun 11, 2013

I’m not sure these aliases are necessary but I prefer :context to :group.

@cupakromer
Copy link
Member

@cupakromer cupakromer commented Jun 13, 2013

No aliases.

Personally, I find the aliases to be more confusing. I may be biased from using :each and seeing :all for so long, but, to me they make a lot of sense.

If aliases are added, I would prefer :example and :context.

My reasoning boils down to the current DSL. example is already an alias for it. And context is starting to gain more usage.

:spec doesn't fit within the current DSL. Though it does match the name we've given to our tests when talking about them. I could also see it being a short reference to the other it alias specify. Though I've rarely seen specify used in practice. Based on this, I feel trying to mix a name (:spec) with a DSL keyword (:context) would just be a mind bender. Better to keep similar references by matching :spec with :group.

@JonRowe
Copy link
Member

@JonRowe JonRowe commented Jun 13, 2013

example it and specify are all aliases for creating an Example they have equal priority at the moment however internally we consider it and specify to be aliases of example with specify being a legacy hangaround.

I've changed my mind and consider example and context to be the preferred additional labels here.

@penelopezone
Copy link
Member

@penelopezone penelopezone commented Sep 1, 2013

@myronmarston @JonRowe this one's gotten properly stale. My feeling based on this is that the conclusion was example and context were the right names if the aliases were added, and that there's an open question as to whether we should add these aliases. Thoughts?

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Sep 1, 2013

@samphippen -- I'm still mulling it over a bit. I almost closed this a while back as there wasn't real consensus; however, recently I've seen a couple situations where people were confused about this:

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

In the context of RSpec.configure, the :all scope is a bit confusing: given it's not in an example group, it sounds like it adds a hook that runs once, before all examples, but really it adds a before(:all) hook to each top-level example group. before(:suite) is the scope that runs before all examples once.

Given that, I'm now thinking that we should add the aliases, and example and context seem to be the winners. I think that :context is a lot more clear for the config case above than :all, anyway.

@fj
Copy link
Contributor Author

@fj fj commented Sep 2, 2013

I'm happy to update the pull request accordingly if you're on board with changing it to example and context, @myronmarston. The config case you describe is one of the areas that was discussed on the original mailing thread, iirc, and it was the one that personally appealed to me.

@penelopezone
Copy link
Member

@penelopezone penelopezone commented Sep 2, 2013

@fj that'd be great, if you could also rebase against master that'd be

@fj
Copy link
Contributor Author

@fj fj commented Sep 2, 2013

Well, sure. It'd be a a pretty lame commit if it wasn't against a rebased master. :)

@fj
Copy link
Contributor Author

@fj fj commented Sep 11, 2013

@myronmarston Just confirming: are you okay with example and context? If so I'll make a new PR.

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Sep 11, 2013

@myronmarston Just confirming: are you okay with example and context? If so I'll make a new PR.

👍

@penelopezone
Copy link
Member

@penelopezone penelopezone commented Oct 16, 2013

@fj just wanting to bump this. Are you still up for opening a new PR?

@fj
Copy link
Contributor Author

@fj fj commented Nov 12, 2013

Closing this one so that discussion can move to the other PR, #1174.

My mind's a little blown that I opened this original PR 3 years ago...

@fj fj closed this Nov 12, 2013
@myronmarston
Copy link
Member

@myronmarston myronmarston commented Nov 15, 2013

Thanks for sticking with us, @fj :). Sometimes it takes us awhile to come around...

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

You can’t perform that action at this time.