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

Fix gemspec and run transpec #220

Closed
wants to merge 2 commits into from
Closed

Fix gemspec and run transpec #220

wants to merge 2 commits into from

Conversation

petergoldstein
Copy link

  1. Fixed gemspec to load by using 'activesupport'
  2. Updated gemspec to allow more recent versions of RSpec
  3. Ran transpec to update specs to RSpec 3.x syntax

@petergoldstein
Copy link
Author

@avit In my reading of Myron's comments the 'x.should ==' pattern is the one that he's trying to eliminate. So I don't really see the point of doing the conversion halfway. What do you get from that?

The 'x.should ==' pattern:

  1. Requires all objects to be decorated such that they respond to 'should'
  2. Requires the override of expected behavior for '==', which is a fundamental method

At heart, these two issues are what drove the RSpec 3.x syntax change.

Given there's question about doing the overall transition, I'd recommend pulling out the gemspec fix and RSpec version upgrade into a separate PR and leaving the transpec changes open for longer discussion.

@avit
Copy link
Collaborator

avit commented Apr 7, 2014

Yeah, the BasicObject#should is the part that's in question. What I meant was that this might be the better goal:

# ...
describe "next_occurrence" do
  subject { schedule.next_occurrence }

  it { should be_a Time }
  its(:hour) { should == start_time.hour }

  context "on a full moon" do
    # setup using before / let
    it { should == x }
  end
end

This is the ExampleGroup version of should (https://github.com/yujinakayama/transpec#two-types-of-should) which is not going away. I'm not 100% either way, but between the "new" RSpec syntax vs. plain Minitest, I'm starting to lean away from RSpec. I don't expect this test suite will change any time soon, and we do have the rspec development gem locked for "< 3.0" anyway.

Maybe @seejohnrun has a preference to sway this either way.

Yes let's fix the gemspec for sure, I can just cherry-pick that from the other commit instead of bothering with another PR.

@seejohnrun
Copy link
Collaborator

I do like @avit's suggestion here - ie: changing the shoulds longer term
If we're going to be using expect everywhere, we may as well not use RSpec

Definitely a fan of @avit's example as well - it'll take some time to get there but shouldn't be toooo bad :)

This conversion is done by Transpec 1.10.4 with the following command:
    transpec -f

* 583 conversions
    from: obj.should
      to: expect(obj).to

* 439 conversions
    from: == expected
      to: eq(expected)

* 13 conversions
    from: obj.should_not
      to: expect(obj).not_to

* 7 conversions
    from: lambda { }.should_not
      to: expect { }.not_to

* 3 conversions
    from: its(:attr) { }
      to: describe '#attr' do subject { super().attr }; it { } end

* 2 conversions
    from: Klass.any_instance.should_receive(:message)
      to: expect_any_instance_of(Klass).to receive(:message)

* 2 conversions
    from: obj.stub!(:message)
      to: allow(obj).to receive(:message)

* 2 conversions
    from: lambda { }.should
      to: expect { }.to

* 1 conversion
    from: =~ /pattern/
      to: match(/pattern/)

* 1 conversion
    from: obj.should_receive(:message)
      to: expect(obj).to receive(:message)
This conversion is done by Transpec 2.2.1 with the following command:
    transpec -f

* 15 conversions
    from: == expected
      to: eq(expected)

* 15 conversions
    from: obj.should
      to: expect(obj).to

For more details: https://github.com/yujinakayama/transpec#supported-conversions
@petergoldstein
Copy link
Author

Rebased and updated gemspec to lock to an RSpec 2.x version for now.

@seejohnrun
Copy link
Collaborator

Obviously it's been a while here, but expect is definitely the norm now (at least in my world). If you are willing to re-run this I'd love to get it merged, and sorry for doubting it!

@petergoldstein
Copy link
Author

I don't think a 2+ year old PR is a good basis for a patch, so I'm closing this PR.

I don't use this gem anymore, so I'm not the best choice for coming up with a replacement PR. But you should be able to run transpec on an updated master and get a correctly updated result.

@seejohnrun
Copy link
Collaborator

seejohnrun commented Jan 15, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants