Skip to content

Conversation

@graceyim
Copy link

Tidying up the language library. Matching functionality of toopher-python library.

@trdarr
Copy link

trdarr commented Nov 19, 2014

Looks good.

Copy link
Member

Choose a reason for hiding this comment

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

Should we include an options hash for extensibility? I believe it would look something like this:

def pair_sms(phone_number, user_name, phone_country='', options={})
  parameters = {
    'phone_number' => phone_number,
    'user_name' => user_name
  }
  phone_country.empty? or (parameters['phone_country'] = phone_country)
  parameters.merge(options)
  return PairingStatus.new(post('pairings/create/sms', parameters))
end

Copy link
Member

Choose a reason for hiding this comment

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

I like that, but we should use **options instead of options = {}

Copy link
Author

Choose a reason for hiding this comment

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

it would need to be either

  parameters.merge!(options)
  return PairingStatus.new(post('pairings/create/sms', parameters))

or

  return PairingStatus.new(post('pairings/create/sms', parameters.merge(options))

Copy link
Member

Choose a reason for hiding this comment

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

Ruby doesn't have **kwargs -- the equivalent for Ruby 1.9 is the options hash ((Ruby 2.0 added better support for keyword args)[http://robots.thoughtbot.com/ruby-2-keyword-arguments]). We've also used the options hash in the pair method above pair_sms.

Copy link

Choose a reason for hiding this comment

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

👍 to options for consistency with pair. Please use merge instead of merge!.

@dshafer
Copy link
Member

dshafer commented Nov 19, 2014

:shipit:

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.

5 participants