Enabling typing in Homebrew::API module #14623
Conversation
| @@ -15,12 +15,10 @@ module API | |||
|
|
|||
| extend Cachable | |||
|
|
|||
| module_function | |||
There was a problem hiding this comment.
I admittedly don't like module_fuction, but I also don't think the functions below actually work as private instance methods, they rely on extend Cachable, which only makes methods available at the class level, iiuc.
| HOMEBREW_CACHE_API = (HOMEBREW_CACHE/"api").freeze | ||
|
|
||
| sig { params(endpoint: String).returns(Hash) } | ||
| def fetch(endpoint) | ||
| def self.fetch(endpoint) |
There was a problem hiding this comment.
Note that this is a style deviation from the files under Library/Homebrew/api/, which use the class << self syntax to define class methods. I prefer the style here, as it makes it obvious at the def where the method is being defined, but I'm equally happy to match the preferred style.
| @@ -37,8 +35,8 @@ def fetch(endpoint) | |||
| raise ArgumentError, "Invalid JSON file: #{Tty.underline}#{api_url}#{Tty.reset}" | |||
| end | |||
|
|
|||
| sig { params(endpoint: String, target: Pathname).returns(Hash) } | |||
| def fetch_json_api_file(endpoint, target:) | |||
| sig { params(endpoint: String, target: Pathname).returns(T.any(Array, Hash)) } | |||
There was a problem hiding this comment.
I usually use T.untyped for methods that return parsed JSON (as does sorbet itself), but was able to get tests to pass with a slight extension of the existing sig here.
| @@ -61,7 +59,7 @@ def fetch_json_api_file(endpoint, target:) | |||
| begin | |||
| begin | |||
| args = curl_args.dup | |||
| args.prepend("--time-cond", target) if target.exist? && !target.empty? | |||
| args.prepend("--time-cond", target.to_s) if target.exist? && !target.empty? | |||
There was a problem hiding this comment.
I believe it is safe to explicitly stringify the Pathname object here, but another option would be to define args as a mixed Array of String and Pathname objects.
dduugg
force-pushed
the
type-api
branch
2 times, most recently
from
dc8b473 to
f6f3d09
Compare
|
The rest of this LGTM |
|
Thanks again @dduugg! |
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?This enables typing for all the files that I could find under
Homebrew::API