fix: renamed #get_without_prefix to #get_url for clarity#11
Conversation
There was a problem hiding this comment.
Pull request overview
This PR renames the #get_without_prefix method to #get_url for improved clarity, adds URL validation to ensure only HTTP/HTTPS schemes are accepted, and includes comprehensive test coverage for the renamed method.
- Renamed method from
#get_without_prefixto#get_urlwith URL validation - Added URI parsing and scheme validation (http/https only)
- Added test coverage for successful URL fetching and invalid URL scenarios
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| lib/kalshi/client.rb | Renamed method to #get_url, added URL validation with scheme checking and error handling |
| test/kalshi/client_test.rb | Added comprehensive test suite for #get_url covering success case and invalid URL scenarios |
| Gemfile.lock | Version bumped from 0.0.3 to 0.0.4 |
| it 'raises Error for invalid URL scheme' do | ||
| client = Rubyists::Kalshi::Client.new | ||
| error = assert_raises(Rubyists::Kalshi::Error) do | ||
| client.get_url('ftp://example.com') | ||
| end | ||
| assert_match(/Invalid URL/, error.message) | ||
| end | ||
|
|
||
| it 'raises Error for invalid URL format' do | ||
| client = Rubyists::Kalshi::Client.new | ||
| error = assert_raises(Rubyists::Kalshi::Error) do | ||
| client.get_url('not_a_url') | ||
| end | ||
| assert_match(/Invalid URL/, error.message) | ||
| end |
There was a problem hiding this comment.
Following BetterSpecs guidelines, use context blocks to group related test cases by scenario. Consider wrapping these validation tests in a 'context "with invalid URLs"' block to make the test organization clearer.
| # Get response from a URL | ||
| # Must pass a full URL, including scheme (http/https), host, etc. | ||
| # | ||
| # @param path [String] The full URL path |
There was a problem hiding this comment.
The parameter is named 'path' but the method now expects a full URL. The parameter name should be 'url' to match the method name and its documentation.
| # @param path [String] The full URL path | |
| # @param url [String] The full URL |
| raise ArgumentError, 'URL must be http or https' unless %w[http https].include?(uri.scheme) | ||
|
|
||
| response = @http.get(url, params:) | ||
| handle_response(response) | ||
| rescue ArgumentError => e |
There was a problem hiding this comment.
The error raised is ArgumentError but caught and re-raised as Error. This creates inconsistency - URI.parse raises URI::InvalidURIError for invalid URIs, not ArgumentError. The custom ArgumentError on line 43 won't be caught by the rescue on line 47 if URI.parse fails first. Consider rescuing URI::InvalidURIError and StandardError to handle both validation scenarios properly.
| raise ArgumentError, 'URL must be http or https' unless %w[http https].include?(uri.scheme) | |
| response = @http.get(url, params:) | |
| handle_response(response) | |
| rescue ArgumentError => e | |
| raise URI::InvalidURIError, 'URL must be http or https' unless %w[http https].include?(uri.scheme) | |
| response = @http.get(url, params:) | |
| handle_response(response) | |
| rescue URI::InvalidURIError => e |
| # Get response from a URL | ||
| # Must pass a full URL, including scheme (http/https), host, etc. | ||
| # | ||
| # @param path [String] The full URL path |
There was a problem hiding this comment.
The documentation says '@param path [String] The full URL path' but the parameter represents a complete URL, not a path. It should read '@param url [String] The full URL' to accurately describe what's expected.
| # @param path [String] The full URL path | |
| # @param url [String] The full URL |
| end | ||
|
|
||
| describe '#get_url' do | ||
| it 'fetches from a full URL' do |
There was a problem hiding this comment.
Following BetterSpecs guidelines, test descriptions should focus on behavior rather than implementation. Consider renaming to 'returns data from a full URL' to describe what the method accomplishes rather than how it works.
1 participant
No description provided.