-
Notifications
You must be signed in to change notification settings - Fork 9
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
Made api more Ruby-like and fixed some bugs. #4
base: master
Are you sure you want to change the base?
Conversation
* Replace endpoint calls with new endpoint names * Comment out community features that are being overhauled
Upgrade to use the upcoming v2 API.
… support for unserialization
…ing-master Conflicts: README.md lib/shopsense/version.rb
|
||
return call_api( __method__, args) | ||
def products(opts = {}) | ||
raise "Interface changed. Use as :query => 'search'" if opts.kind_of? String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
return nil unless opts.kind_of?(Hash)
I'd prefer not to raise an error. Perhaps incorporate a logger instance and log a WARN
describing the malformed arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method signature changed,s o client code would have to be updated. I think it would be rather confusing to return nil. If you prefer to maintain BC, you could change it to:
opts = {:search => opts} if opts.kind_of? String
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't really consider that backward compatibility since the name of the method is changing but a nice feature and a good alternative for raising an error.
@http_keep_alive = args['http_keep_alive'].nil? ? true : args['http_keep_alive'] | ||
@site = args['site'] || 'www.shopstyle.com' | ||
@http_session = {} | ||
end | ||
|
||
# Searches the shopsense API | ||
# @param [String] search_string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the doc to reflect the opts
param change.
Hey. Sorry for the silence - I've been on holidays and is generally way too busy. I'll see if I can find the time to look at your comments within the next couple of days. |
These are some changes that I made a while back. I thought the api was maintained by shopstyle, so I submitted the PR to the wrong fork. Here it is again.