keolo / mixpanel_client

Ruby interface to the Mixpanel Data API
MIT License
148 stars 72 forks source link

Avoid overriding the arg of client.request #24

Closed yaotti closed 10 years ago

yaotti commented 11 years ago

Overriding it can cause unexpecting errors in continuous requests.

keolo commented 11 years ago

Could you write a spec for this?

yaotti commented 11 years ago

Sorry, I'll add a spec.

But it seems difficult to write a spec for the bug. How do you think the spec should be? The bug I fixed in the pull request is as follows:

args = { type: 'general' }
client.request('events/top', args) # => return correct data
client.request('events/top', args) # => Invalid API signature (Mixpanel::HTTPError)

I think testing the immutability of args is ridiculous like this:

args = { type: 'general' }
client.request('events/top', args)
args.should == { type: 'general' }

However, I have no idea how to validate the API signature in specs. Could you give me some advices?

jcf commented 10 years ago

RSpec has change expectations that might be useful here…

let(:options) { {some: 'hash'} }

it 'does not modify the provided options' do
  expect { client.request('events/top', options) }.to_not change(options)
end

HTH.

yaotti commented 10 years ago

@jcf thanks, that sample was so useful to me!

@keolo I added a spec so please take a look at this PR :octocat:

keolo commented 10 years ago

Thanks. I've been pretty busy but I'll take a look and merge it in.

keolo commented 10 years ago

Your pull request has been included in v3.1.1. Thank you for your contribution!

yaotti commented 10 years ago

Thank you for the merge and providing the useful gem! :smiley: