Conversation
|
Maybe we should call this |
| # @param [String|Symbol, nil] output response format using the SerpApi output parameter [optional] | ||
| # @return [String|Hash] raw HTML, Markdown, or JSON / Hash | ||
| def search_archive(search_id, format = :json, output: nil) | ||
| format = output.to_s.to_sym unless output.nil? |
There was a problem hiding this comment.
Why separate format and output arguments?
Personally I prefer |
|
Thanks @trusche for having a look. You are right on search_archive, we should cleanly just pass the extension. I fixed it.
Well, that's how I started as well :) However, consistency among our clients is more important I think (already in Python and PHP libs) and I think there is a tiny advantage of matching the extension. |
|
Ruby and rails offer a lot of syntactic sugar, we could at least create an alias? |
|
I was thinking about it, but on the other hand I want us to be direct in docs and want to send people to one method everywhere. So creating an alias just on its own doesn't feel that good to me.
Do you think it's still worth it? |
|
@strzibny Thanks for the efforts. Functionality wise everything looks good to me 👍 One minor thing I observed (not due to code changes as part of this PR) is that when I pass I feel it would be good to mention some sample code in our Readme doc. Something like below where we are differentiating the three, # For JSON output
result_json = serpapi_client.search
# For HTML output
result_html = serpapi_client.html
# For Markdown output
result_md = serpapi_client.mdNote - I observe that our python library supports |
|
Thanks @pulkitchowdry, you are right, we should have already been choosing the right decoder for HTML. I included it in this change. |
This is WIP change for Markdown support.