-
-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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 some utils/
files to Sorbet typed: strict
#19094
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.
Good to go whenever you are! All comments are non-blocking, we can always clean this up post merge.
@@ -97,8 +97,8 @@ def pypi_info(new_version: nil) | |||
sig { returns(String) } | |||
def to_s | |||
if valid_pypi_package? | |||
out = name | |||
out += "[#{extras.join(",")}]" if extras.present? | |||
out = T.must(name) |
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.
If we're doing a T.must
here feels like it might be nice to enforce in def name
and def initialize
that it's a String
type instead?
tcsh: "~/.tcshrc", | ||
zsh: "~/.zshrc", | ||
}.freeze, | ||
T::Hash[T.nilable(Symbol), 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.
T::Hash[T.nilable(Symbol), String], | |
T::Hash[Symbol, String], |
feels more appropriate?
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.
Tried that, but in def profile
’s SHELL_PROFILE_MAP.fetch(preferred, "~/.profile")
, preferred
can be nil
and there’s no default fallback.
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.
Does that produce a type-error? Feels like it might be nicer in that case to have an early return
rather than attempting a fetch
with nil
. Thoughts?
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.
Yes. And good idea, yeah, I'll do that.
Co-authored-by: Mike McQuaid <[email protected]>
7a069dd
to
4941137
Compare
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.