-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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.
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
Bump more files to Sorbet typed: strict
#18354
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @issyl0! A few style suggestions, none blocking.
@@ -38,7 +39,9 @@ def migrate(dry_run: false) | |||
old_caskroom_path = old_cask.caskroom_path | |||
new_caskroom_path = new_cask.caskroom_path | |||
|
|||
old_installed_caskfile = old_cask.installed_caskfile.relative_path_from(old_caskroom_path) | |||
return if (old_caskfile = old_cask.installed_caskfile).nil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return if (old_caskfile = old_cask.installed_caskfile).nil? | |
old_caskfile = old_cask.installed_caskfile | |
return if old_caskfile.nil? |
Don't feel strongly but this format makes more sense to me when the variable's reused below so the definition is not "hidden".
cmd: T.nilable(String), | ||
empty_argv: T::Boolean, | ||
usage_error: T.nilable(String), | ||
remaining_args: T.nilable(T::Array[String]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remaining_args: T.nilable(T::Array[String]) | |
remaining_args: T::Array[String]) |
feels nicer if possible given the default parameter is []
params( | ||
cmd: String, | ||
path: Pathname, | ||
remaining_args: T.nilable(T::Array[String]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remaining_args: T.nilable(T::Array[String]), | |
remaining_args: T::Array[String], |
feels nicer if possible given there's no default parameter
sig { | ||
params( | ||
path: Pathname, | ||
remaining_args: T.nilable(T::Array[String]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remaining_args: T.nilable(T::Array[String]) | |
remaining_args: T::Array[String] |
|
||
# Try parsing arguments here in order to show formula options in help output. | ||
cmd_parser.parse(remaining_args, ignore_invalid_options: true) | ||
cmd_parser.generate_help_text | ||
end | ||
private_class_method :parser_help | ||
|
||
sig { params(path: Pathname).returns(T::Array[T.nilable(String)]) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sig { params(path: Pathname).returns(T::Array[T.nilable(String)]) } | |
sig { params(path: Pathname).returns(T::Array[String]) } |
def self.command_help_lines(path) | ||
path.read | ||
.lines | ||
.grep(/^#:/) | ||
.map { |line| line.slice(2..-1).delete_prefix(" ") } | ||
.map { |line| line.slice(2..-1)&.delete_prefix(" ") } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.map { |line| line.slice(2..-1)&.delete_prefix(" ") } | |
.filter_map { |line| line.slice(2..-1)&.delete_prefix(" ") } |
means it'll be T::Array[String]
sig { params(file: T.any(Pathname, String), objdump: String).returns(T::Boolean) } | ||
def cpuid_instruction?(file, objdump = "objdump") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sig { params(file: T.any(Pathname, String), objdump: String).returns(T::Boolean) } | |
def cpuid_instruction?(file, objdump = "objdump") | |
sig { params(file: T.any(Pathname, String), objdump: Pathname).returns(T::Boolean) } | |
def cpuid_instruction?(file, objdump) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Pathname
part is actually necessary since callers above pass objdump
as a Pathname
.
def cpuid_instruction?(file, objdump = "objdump") | ||
@instruction_column_index ||= {} | ||
@instruction_column_index ||= T.let({}, T.nilable(T::Hash[String, T.untyped])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@instruction_column_index ||= T.let({}, T.nilable(T::Hash[String, T.untyped])) | |
@instruction_column_index ||= T.let({}, T.nilable(T::Hash[String, Integer])) |
I think?
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?typed: strict
in all (non-package) files in Homebrew organisation #17297.