Fix timeout & symbolize_names in default#22
Closed
0xGhostCAT wants to merge 1 commit into
Closed
Conversation
First, thanks for maintaining this gem — it's clean and pleasant to read. I'll be upfront: I'm a "vibe coder" I'm not a deep Ruby-internals expert; what I'm good at is spotting ideas, refining them, and testing them carefully. While studying serpapi-ruby I noticed a handful of small-but-real issues, drafted fixes (with AI assistance), and then verified everything both offline and against the live API. It genuinely behaves better now, so I thought I'd share it. Please treat this as a proposal: merge it, cherry-pick parts, adapt it to your conventions, or pass entirely no hard feelings. I'm happy to split it into smaller PRs or adjust anything to match how you like things done. What this changes Bug fixes (all affect the default configuration) timeout was ignored in persistent mode. Persistent is the default, but the socket was built with HTTP.persistent(url) with no timeout attached — the timeout was only applied on the non-persistent path. So the documented timeout: option effectively did nothing for most users, and a stalled connection could hang indefinitely. → now HTTP.timeout(@timeout).persistent(url) symbolize_names: set on the constructor was ignored. It was read from per-call params only, so Client.new(symbolize_names: false) had no effect unless repeated on every search. Now resolved with precedence: per-call → constructor → default. The constructor mutated the caller's hash. It deleted :timeout / :persistent from the passed-in hash before cloning. Now it works on a private copy and never touches your object. html / search_archive(:html) didn't drain the persistent socket. The JSON path calls response.flush; the HTML path returned the lazy body without it. Now it reads the body to a String and flushes, matching the JSON path. Feature - addresses Added a raise_on_search_error option. Transport errors (non‑200) always raise; a 200 carrying an error field raises only when opted in. search_archive defaults to raise_on_search_error: false, so an archived search that recorded an error (e.g. "Google hasn't returned any results") can be read instead of raised - which is exactly what #17 asks for. search / html keep their current raising behavior, so nothing breaks. ```ruby # inspect a failed archived search without rescuing result = client.search_archive(search_id) # returns the payload (incl. :error) # or opt back into the old behavior: client.search_archive(search_id, raise_on_search_error: true) ``` Performance `query()` built the request hash with `@params.clone.merge(params).compact` — three Hash allocations per request, and the `clone` is redundant since `merge` doesn't mutate the receiver. Reduced to a single allocation (`merge` + in-place `compact!`). Small, but it's on the hot path for the "search at scale" use case. Tests & CI Added an offline test suite (spec/offline, using WebMock): 13 specs that run with no API key and no network, plus a rake offline task. This locks down each fix and lets contributors run tests on a fork for free (today every spec needs a live, billed key). Reworked CI: lint + offline matrix across Ruby 2.7 → 3.4 (no secret, runs on PRs/forks), with the live tests gated on SERPAPI_KEY. Bumped actions/checkout@v4 and ruby/setup-ruby@v1. Packaging / hygiene Allowed HTTP.rb 6.x (>= 5.2, < 7.0) ~> 5.2 blocked the current major. Verified on both 5.3.1 and 6.0.3. Added gemspec metadata (source_code_uri, changelog_uri, bug_tracker_uri, documentation_uri, rubygems_mfa_required). Added .gitattributes (eol=lf) + Layout/EndOfLine: lf for deterministic cross-platform lint; removed a MethodLength RuboCop warning (missing Metrics/ department); dropped a stale # faraday comment. Bumped to 1.1.0 and updated CHANGELOG + README. Backward compatibility Existing behavior is preserved: a non‑200 (e.g. the Missing query \q` parameter.` 400) still raises with the same message; search / html still raise on a 200 error by default; bad-decoder / bad-endpoint / inspect-masking specs are unchanged. The only intentional behavior change is that search_archive no longer raises on an archived error by default (opt back in with raise_on_search_error: true).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First, thanks for maintaining this gem — it's clean and pleasant to read.
I'll be upfront: I'm a "vibe coder" I'm not a deep Ruby-internals expert; what I'm good at is spotting ideas, refining them, and testing them carefully. While studying serpapi-ruby I noticed a handful of small-but-real issues, drafted fixes (with AI assistance), and then verified everything both offline and against the live API. It genuinely behaves better now, so I thought I'd share it.
Please treat this as a proposal: merge it, cherry-pick parts, adapt it to your conventions, or pass entirely no hard feelings. I'm happy to split it into smaller PRs or adjust anything to match how you like things done.
What this changes
Bug fixes (all affect the default configuration)
timeout was ignored in persistent mode. Persistent is the default, but the socket was built with HTTP.persistent(url) with no timeout attached — the timeout was only applied on the non-persistent path. So the documented timeout: option effectively did nothing for most users, and a stalled connection could hang indefinitely. → now HTTP.timeout(@timeout).persistent(url)
symbolize_names: set on the constructor was ignored. It was read from per-call params only, so Client.new(symbolize_names: false) had no effect unless repeated on every search. Now resolved with precedence: per-call → constructor → default.
The constructor mutated the caller's hash. It deleted :timeout / :persistent from the passed-in hash before cloning. Now it works on a private copy and never touches your object.
html / search_archive(:html) didn't drain the persistent socket. The JSON path calls response.flush; the HTML path returned the lazy body without it. Now it reads the body to a String and flushes, matching the JSON path.
Feature - addresses
Added a raise_on_search_error option. Transport errors (non‑200) always raise; a 200 carrying an error field raises only when opted in. search_archive defaults to raise_on_search_error: false, so an archived search that recorded an error (e.g. "Google hasn't returned any results") can be read instead of raised - which is exactly what #17 asks for. search / html keep their current raising behavior, so nothing breaks.
Performance
query()built the request hash with@params.clone.merge(params).compact— three Hash allocations per request, and thecloneis redundant sincemergedoesn't mutate the receiver. Reduced to a single allocation (merge+ in-placecompact!). Small, but it's on the hot path for the "search at scale" use case.Tests & CI
Added an offline test suite (spec/offline, using WebMock): 13 specs that run with no API key and no network, plus a rake offline task. This locks down each fix and lets contributors run tests on a fork for free (today every spec needs a live, billed key). Reworked CI: lint + offline matrix across Ruby 2.7 → 3.4 (no secret, runs on PRs/forks), with the live tests gated on SERPAPI_KEY. Bumped actions/checkout@v4 and ruby/setup-ruby@v1.
Packaging / hygiene
Allowed HTTP.rb 6.x (>= 5.2, < 7.0) ~> 5.2 blocked the current major. Verified on both 5.3.1 and 6.0.3. Added gemspec metadata (source_code_uri, changelog_uri, bug_tracker_uri, documentation_uri, rubygems_mfa_required). Added .gitattributes (eol=lf) + Layout/EndOfLine: lf for deterministic cross-platform lint; removed a MethodLength RuboCop warning (missing Metrics/ department); dropped a stale # faraday comment. Bumped to 1.1.0 and updated CHANGELOG + README.
Backward compatibility
Existing behavior is preserved:
a non‑200 (e.g. the Missing query \q
parameter.400) still raises with the same message; search / html still raise on a 200 error by default; bad-decoder / bad-endpoint / inspect-masking specs are unchanged. The only intentional behavior change is that search_archive no longer raises on an archived error by default (opt back in with raise_on_search_error: true).