Skip to content

Conversation

@troelskn
Copy link

@troelskn troelskn commented Jul 7, 2014

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.

Copy link
Member

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.

Copy link
Author

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

Copy link
Member

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.

Copy link
Member

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.

@troelskn
Copy link
Author

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.

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.

3 participants