Some small changes before 1.6.0 release #85

Merged
merged 3 commits into from Feb 12, 2013

Conversation

Projects
None yet
2 participants
@rwz
Member

rwz commented Feb 12, 2013

No description provided.

@rwz

This comment has been minimized.

Show comment
Hide comment
@rwz

rwz Feb 12, 2013

Member

Just put rescue nil back

Member

rwz commented Feb 12, 2013

Just put rescue nil back

@rwz

This comment has been minimized.

Show comment
Hide comment
@rwz

rwz Feb 12, 2013

Member

We had two separate blocks that began with if adapter == 'json_gem' || adapter == 'json_pure'

I've only put them into one.

Member

rwz commented Feb 12, 2013

We had two separate blocks that began with if adapter == 'json_gem' || adapter == 'json_pure'

I've only put them into one.

@rwz

This comment has been minimized.

Show comment
Hide comment
@rwz

rwz Feb 12, 2013

Member

The whole block looks like this afterwards:

if adapter == 'json_gem' || adapter == 'json_pure'
  describe 'with :pretty option set to true' do
    it 'passes default pretty options' do
      ::JSON.should_receive(:generate).with(['foo'], JSON::PRETTY_STATE_PROTOTYPE.to_h).and_return('["foo"]')
      MultiJson.dump('foo', :pretty => true)
    end
  end

  describe 'with :indent option' do
    it 'passes it on dump' do
      ::JSON.should_receive(:generate).with(['foo'], {:indent => "\t"}).and_return('["foo"]')
      MultiJson.dump('foo', :indent => "\t")
    end
  end

  describe 'with :quirks_mode option' do
    it 'passes it on load' do
      ::JSON.should_receive(:parse).with('["foo"]', {:quirks_mode => true, :create_additions => false}).and_return(['foo'])
      MultiJson.load('"foo"', :quirks_mode => true)
    end
  end
end

I think it makes perfect sense to group options pasing for json/json_pure here. Actually, we could even move this part to multi_json_spec instead of shared_adapter_spec.

Member

rwz commented Feb 12, 2013

The whole block looks like this afterwards:

if adapter == 'json_gem' || adapter == 'json_pure'
  describe 'with :pretty option set to true' do
    it 'passes default pretty options' do
      ::JSON.should_receive(:generate).with(['foo'], JSON::PRETTY_STATE_PROTOTYPE.to_h).and_return('["foo"]')
      MultiJson.dump('foo', :pretty => true)
    end
  end

  describe 'with :indent option' do
    it 'passes it on dump' do
      ::JSON.should_receive(:generate).with(['foo'], {:indent => "\t"}).and_return('["foo"]')
      MultiJson.dump('foo', :indent => "\t")
    end
  end

  describe 'with :quirks_mode option' do
    it 'passes it on load' do
      ::JSON.should_receive(:parse).with('["foo"]', {:quirks_mode => true, :create_additions => false}).and_return(['foo'])
      MultiJson.load('"foo"', :quirks_mode => true)
    end
  end
end

I think it makes perfect sense to group options pasing for json/json_pure here. Actually, we could even move this part to multi_json_spec instead of shared_adapter_spec.

@sferik

This comment has been minimized.

Show comment
Hide comment
@sferik

sferik Feb 12, 2013

Member

Okay, that was hard to see from the diff.

Still, I think it doesn't make sense to have a load test inside describe '.dump'.

Member

sferik commented Feb 12, 2013

Okay, that was hard to see from the diff.

Still, I think it doesn't make sense to have a load test inside describe '.dump'.

@rwz

This comment has been minimized.

Show comment
Hide comment
@rwz

rwz Feb 12, 2013

Member

There is no load test inside describe dump.

Member

rwz commented Feb 12, 2013

There is no load test inside describe dump.

@rwz

This comment has been minimized.

Show comment
Hide comment
@rwz

rwz Feb 12, 2013

Member

Ah, wait, there is. My bad, I didn't notice

Member

rwz commented Feb 12, 2013

Ah, wait, there is. My bad, I didn't notice

@sferik

This comment has been minimized.

Show comment
Hide comment
@sferik

sferik Feb 12, 2013

Member

describe 'with :quirks_mode option' tests the MultiJson.load method.

It appears inside a describe '.dump' context.

Member

sferik commented Feb 12, 2013

describe 'with :quirks_mode option' tests the MultiJson.load method.

It appears inside a describe '.dump' context.

@rwz

This comment has been minimized.

Show comment
Hide comment
@rwz

rwz Feb 12, 2013

Member

What do you think about moving this whole params passing spec block outside of descibe dump?

Member

rwz commented Feb 12, 2013

What do you think about moving this whole params passing spec block outside of descibe dump?

@sferik

This comment has been minimized.

Show comment
Hide comment
@sferik

sferik Feb 12, 2013

Member

I don't know. I'm trying to do this: andreareginato/betterspecs#2

If you want to restructure the specs in a way that makes more sense to you, I'm okay with that, but I'm also okay with duplicating the line:

if adapter == 'json_gem' || adapter == 'json_pure'

No big deal, either way. It's just a stylistic thing. I just want it to be consistent and make sense.

Member

sferik commented Feb 12, 2013

I don't know. I'm trying to do this: andreareginato/betterspecs#2

If you want to restructure the specs in a way that makes more sense to you, I'm okay with that, but I'm also okay with duplicating the line:

if adapter == 'json_gem' || adapter == 'json_pure'

No big deal, either way. It's just a stylistic thing. I just want it to be consistent and make sense.

@rwz

This comment has been minimized.

Show comment
Hide comment
@rwz

rwz Feb 12, 2013

Member

Ok, gimme 5 minutes, I'll try to factor all json/json_pure out nicely. If you won't like it, I'll just remove this commit :)

Member

rwz commented Feb 12, 2013

Ok, gimme 5 minutes, I'll try to factor all json/json_pure out nicely. If you won't like it, I'll just remove this commit :)

@rwz

This comment has been minimized.

Show comment
Hide comment
@rwz

rwz Feb 12, 2013

Member

Ok, check this: rwz/multi_json@f5c4f54

Does it make any sense for you?

Member

rwz commented Feb 12, 2013

Ok, check this: rwz/multi_json@f5c4f54

Does it make any sense for you?

@sferik

This comment has been minimized.

Show comment
Hide comment
@sferik

sferik Feb 12, 2013

Member

Yes, this is much better. Don't you agree?

Member

sferik commented Feb 12, 2013

Yes, this is much better. Don't you agree?

@rwz

This comment has been minimized.

Show comment
Hide comment
@rwz

rwz Feb 12, 2013

Member

Yep, I think so too.

Member

rwz commented Feb 12, 2013

Yep, I think so too.

sferik added a commit that referenced this pull request Feb 12, 2013

Merge pull request #85 from rwz/master
Some small changes before 1.6.0 release

@sferik sferik merged commit 0326901 into intridea:master Feb 12, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment