Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enabling typing in Homebrew::API module #14623

Merged
merged 1 commit into from
Feb 14, 2023
+17 −12
Merged

Enabling typing in Homebrew::API module #14623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 7 additions & 9 deletions Library/Homebrew/api.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# typed: false
# typed: true
# frozen_string_literal: true

require "api/analytics"
Expand All @@ -15,12 +15,10 @@ module API

extend Cachable

module_function
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return cache[endpoint] if cache.present? && cache.key?(endpoint)

api_url = "#{Homebrew::EnvConfig.api_domain}/#{endpoint}"
Expand All @@ -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)) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

def self.fetch_json_api_file(endpoint, target:)
retry_count = 0
url = "#{Homebrew::EnvConfig.api_domain}/#{endpoint}"
default_url = "#{HOMEBREW_API_DEFAULT_DOMAIN}/#{endpoint}"
Expand All @@ -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?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

unless skip_download
ohai "Downloading #{url}" if $stdout.tty?
# Disable retries here, we handle them ourselves below.
Expand Down Expand Up @@ -97,7 +95,7 @@ def fetch_json_api_file(endpoint, target:)
end

sig { params(filepath: String, repo: String, git_head: T.nilable(String)).returns(String) }
def fetch_file_source(filepath, repo:, git_head: nil)
def self.fetch_file_source(filepath, repo:, git_head: nil)
git_head ||= "master"
endpoint = "#{git_head}/#{filepath}"
return cache[endpoint] if cache.present? && cache.key?(endpoint)
Expand All @@ -110,7 +108,7 @@ def fetch_file_source(filepath, repo:, git_head: nil)
end

sig { params(json: Hash).returns(Hash) }
def merge_variations(json)
def self.merge_variations(json)
if (bottle_tag = ::Utils::Bottles.tag.to_s.presence) &&
(variations = json["variations"].presence) &&
(variation = variations[bottle_tag].presence)
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/api/analytics.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# typed: false
# typed: true
# frozen_string_literal: true

module Homebrew
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/api/cask.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# typed: false
# typed: true
# frozen_string_literal: true

module Homebrew
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/api/formula.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# typed: false
# typed: true
# frozen_string_literal: true

module Homebrew
Expand Down
7 changes: 7 additions & 0 deletions Library/Homebrew/env_config.rbi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# typed: strict

module Homebrew::EnvConfig
# This is necessary due to https://github.com/sorbet/sorbet/issues/6726
sig { returns(String) }
def self.api_auto_update_secs; end
end