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

Deprecate 'its' in favor of rspec-its gem as part of #1083 #1104

Merged
merged 1 commit into from
Oct 10, 2013
Merged

Deprecate 'its' in favor of rspec-its gem as part of #1083 #1104

merged 1 commit into from
Oct 10, 2013

Conversation

palfvin
Copy link
Contributor

@palfvin palfvin commented Oct 7, 2013

Wasn't sure about wording and assumed that no test was required for deprecation. In support of #1083

@@ -455,6 +455,7 @@ def subject!(name=nil, &block)
# its(:age) { should eq(25) }
# end
def its(attribute, &block)
RSpec.deprecate('Use of rspec-core\'s "its" method', :replacement => 'rspec-its gem')
Copy link
Member

Choose a reason for hiding this comment

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

We typically do spec these, just to make sure we get the call trace right (we've had so many bugs in the past).

See https://github.com/rspec/rspec-core/blob/master/spec/support/helper_methods.rb#L43

@JonRowe
Copy link
Member

JonRowe commented Oct 8, 2013

Thanks, we like to spec these things though. We'd also want to check the call site is correct, (even though it's auto generated). There's some helpers for this. spec/support/helper_methods.rb#L26-48

@JonRowe
Copy link
Member

JonRowe commented Oct 8, 2013

In which @xaviershay and I once again say the same thing within a few minutes of each other...

@palfvin
Copy link
Contributor Author

palfvin commented Oct 8, 2013

No problem, will do, although I'm curious how defects could creep in since you introduced the deprecate method. While I'm at it, is the minimalist deprecation text sufficient? I couldn't find a clear precedent for what it should be.

@JonRowe
Copy link
Member

JonRowe commented Oct 8, 2013

The deprecation text is sufficient, we prefer precise messages like that :)

r.e. defects, the call site isn't always where you'd expect due to the stack... We've had regressions creep in that we've not expected.

@palfvin
Copy link
Contributor Author

palfvin commented Oct 9, 2013

Hi guys, in case it wasn't clear, that last commit of mine was my cut at the spec for the deprecation. :-)

@myronmarston
Copy link
Member

@palfvin -- thanks, we'll take a look. Github doesn't notify us when new commits are pushed so if you want to comment again, you need to post a comment (as you did) so we'll get notified.

@@ -348,6 +348,12 @@ def not_ok?; false; end
end

describe "#its" do

it "should issue deprecation warning" do
expect_deprecation_with_call_site(__FILE__, __LINE__+1)
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to assert the right deprecation is being raised, just add a third argument of /"its" method/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method doesn't accept a third argument and the expect_warning_with_call_site which does take a third argument isn't in this branch.

Copy link
Member

Choose a reason for hiding this comment

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

Huh, my bad. Want to add it? :)

@palfvin
Copy link
Contributor Author

palfvin commented Oct 10, 2013

@JonRowe I wasn't sure whether you were talking about bringing the existing warning helper into 2.99 or adding a third argument to expect_deprecation_with_call_site. I assumed the latter and implemented the 3rd argument as a check against the :deprecated string, ignoring the :replacement string. I also haven't submitted anything to master, pending your feedback.

I realize we're now up to three commits for this pull request. Do I need to "squash" these down?

@JonRowe
Copy link
Member

JonRowe commented Oct 10, 2013

If you wouldn't mind I think it would be appropriate here. The helper change is fine.

@palfvin
Copy link
Contributor Author

palfvin commented Oct 10, 2013

Ok, see new, single commit. One nit - I changed the name of the 3rd helper argument to 'deprecated' from 'deprecation' to be more precise, since it's not trying to match the entire deprecation warning.

JonRowe added a commit that referenced this pull request Oct 10, 2013
Deprecate 'its' in favor of rspec-its gem as part of #1083
@JonRowe JonRowe merged commit 0928c69 into rspec:2-99-maintenance Oct 10, 2013
@JonRowe
Copy link
Member

JonRowe commented Oct 10, 2013

Thanks @palfvin :)

@palfvin
Copy link
Contributor Author

palfvin commented Oct 11, 2013

Thank you, Jon, for your patience/support. Realizing that shepherding me through this relatively trivial little extraction probably took more of your time than doing it yourself, is there something else in particular you'd appreciate help with?

@JonRowe
Copy link
Member

JonRowe commented Oct 11, 2013

It's a pleasure to help, one thing that'd be good is to run through the process of upgrading an its based suite from 2.14 to 3.x and comment on #1083 so we can close it off. The process I'd use is:

  • Run suite all passing on 2.14
  • Upgrade to 2.99 see warning.
  • Add rspec-its and see warning disapear
  • Uninstall rspec-its
  • Upgrade to 3.x see failing suite.
  • Add rspec-its and see passing suite.

Also, looking at our open issues (not related to this), we have one open surrounding documentation so if you're looking for easy things that's always a good place to start.

@palfvin
Copy link
Contributor Author

palfvin commented Oct 12, 2013

Ok, I'll do that, but should the gem be registered before we close this issue? If so, is there something we need to wait for before proceeding with the registration?

As for next thing to help on, I'm glad to contribute to the documentation, but I'm not really looking for "easy". ;-) I'm looking for challenging and doable and something that would drive me to come further up to speed on the code.

@JonRowe
Copy link
Member

JonRowe commented Oct 12, 2013

The gem will have been "registered" by Myron, I think the plan is to wait
until 2.99 to release the real ones, @myronmarston?

Also docs are quite useful intro to various parts of RSpec :) but also it
was the one open issue that didn't look like it was super hard or in
progress, welcome to just jump on one.

On Saturday, 12 October 2013, palfvin wrote:

Ok, I'll do that, but should the gem be registered before we close this
issue? If so, is there something we need to wait for before proceeding with
the registration?

As for next thing to help on, I'm glad to contribute to the documentation,
but I'm not really looking for "easy". ;-) I'm looking for challenging and
doable and something that would drive me to come further up to speed on the
code.


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

@myronmarston
Copy link
Member

@palfvin -- I just gave you owner access to rspec-its on rubygems.org, so you can release the gem whenever you want with the relase rake task.

We'll need a final 1.0 release out by the time 2.99/3.0 final comes out, and it'd be nice to have a pre-1.0 release out by the time the first 2.99/3.0 alpha's come out (which we're hoping to release later in october).

@palfvin
Copy link
Contributor Author

palfvin commented Oct 17, 2013

@JonRowe Just wanted to let you know that after pushing the gem to rubygems, I ran through the steps you'd listed above involving testing with various versions.

yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 6, 2021
Deprecate 'its' in favor of rspec-its gem as part of rspec/rspec-core#1083

---
This commit was imported from rspec/rspec-core@0928c69.
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