-
Notifications
You must be signed in to change notification settings - Fork 483
Conversation
978c053 to
8ed225d
Compare
venraom
commented
Sep 25, 2025
@CodiumAI-Agent /improve
QodoAI-Agent
commented
Sep 25, 2025
PR Code Suggestions ✨
|
venraom
commented
Sep 25, 2025
@CodiumAI-Agent /review
kubaw
commented
Oct 17, 2025
Would it be possible to merge this? We're really looking forward to this feature.
QodoAI-Agent
commented
Oct 17, 2025
PR Reviewer Guide 🔍
Here are some key observations to aid the review process:
🎫 Ticket compliance analysis ✅
1375 - PR Code Verified
Compliant requirements:
- Allow configuring a proxy server for all ShopifyAPI HTTP requests.
- Expose a way to pass through HTTParty options without affecting global HTTParty state.
- Provide documentation on how to configure proxy and related HTTP params.
- Add tests to cover the new configuration path.
Requires further human verification:
- Manual verification with a real proxy environment (since WebMock ignores proxy settings).
Possible Issue
Passing Context.httparty_params with ** splat alongside explicit headers/query/body relies on params shape not colliding with HTTParty's expected options. If a user includes any of these keys, behavior could be overridden unintentionally; also ensure nil/empty hash defaults are handled safely.
res = T.cast(HTTParty.send( request.http_method, parsed_uri.to_s, **Context.httparty_params, headers: headers, query: request.query, body: request.body.class == Hash ? T.unsafe(request.body).to_json : request.body, ), HTTParty::Response)
Type Signature
The sig for setup defines httparty_params as a required T::Hash[Symbol, T.untyped], but the default is {}. Consider making it T.nilable(...) or with default keyword to prevent Sorbet mismatch in callers omitting it.
host_name: T.nilable(String), host: T.nilable(String), private_shop: T.nilable(String), user_agent_prefix: T.nilable(String), old_api_secret_key: T.nilable(String), api_host: T.nilable(String), response_as_struct: T.nilable(T::Boolean), rest_disabled: T.nilable(T::Boolean), httparty_params: T::Hash[Symbol, T.untyped] ).void
Docs Accuracy
Example shows http_proxyaddr containing a scheme and port while also setting http_proxyport separately; for HTTParty/Net::HTTP the address should typically be a hostname. Clarify to avoid misconfiguration.
Optionally, you can override HTTP request params such as timeout, debug_output, or proxy settings. ```ruby ShopifyAPI::Context.setup( # other params... httparty_params: { timeout: 60, # Set the open/read/write timeout for HTTP requests open_timeout: 60, # Set the open timeout for HTTP requests read_timeout: 60, # Set the read timeout for HTTP requests write_timeout: 60, # Set the write timeout for HTTP requests debug_output: false, # Set to true to enable debug output for HTTP requests http_proxyaddr: "http://proxy.example.com:8080", # Set the HTTP proxy address http_proxyport: 8080, # Set the HTTP proxy port http_proxyuser: "username", # Set the HTTP proxy username http_proxypass: "password", # Set the HTTP proxy password } )
Allows to override HTTP request params such as timeout, debug_output, or proxy settings.
8ed225d to
bc3a922
Compare
senid231
commented
Jun 11, 2026
PR rebased and code review applied
Uh oh!
There was an error while loading. Please reload this page.
Description
Fixes #1375
Adds the ability to configure a proxy server for all
ShopifyAPIHTTP requestsHow has this been tested?
proxy server configuration can't be tested via
WebMockbecause it ignores proxy settingsChecklist: