-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add types to clients #8028
Add types to clients #8028
Conversation
def get(url) | ||
response = nil | ||
response = T.let(nil, T.nilable(Excon::Response)) |
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.
Are these T.let
s and T.must
s checked at runtime as well, or are these also just compile-time checks?
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.let
and T.must
are both static and runtime checks. Unfortunately, it's not possible to satisfy Sorbet's type checks with just static checks in this specific instance.
Sorbet infers response = nil
is of type nil
, and then complains that Excon::Result
cannot be assigned to type nil
. So response
has to be defined as T.nilable(Excon::Response)
.
Later we make calls to response.status
and response.body
. I made the response.status
calls use the safe navigation operator &.
. But before returning from the method, I assert that response
is not nil
by calling T.must(response)
. The alternative is propagating the nil all the way up the stack, which will require a lot of condditionals where the result of a method call might be 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.
I see, thanks for explaining! It might make sense to enable the runtime checking only in tests and not in prod yet if there's a way to make that configurable? That way we reduce the chance of introducing any new issues, and also I believe there's a (small?) cost incurred when enabling runtime checks?
T::Configuration.default_checked_level = :tests
Should do the trick, I believe
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.
Yeah, I can add that configuration somewhere in the root of common
. There is a small performance penalty for runtime type checks, but it's somewhere in the region of 5%1
we have runtime type checking turned on even for our most performance-sensitive request paths, and the slowdown from runtime type checking amounts to about 5%.
And if we ever end up using the Sorbet compiler to AOT compile dependabot-core
we'll see huge performance increases with runtime type checks2
Footnotes
This one seems like a change that we should test in staging, and deploy before merging, because it's actually changing code ran in prod, as opposed to the previous PRs that were mainly no-ops |
Superseded by #8038 (as I created this from the |
Mainly adding
typed: true
to clients, but also includes:dependabot.rb
andwildcard_matcher.rb
typed: strong
mostly as a learning experience to see what is required at that type enforcement levelto_json
forArray
andHash
AWS
errors which Sorbet is unable to determine (See The TODO RBI file for more details)*_with_retries
clients withT.unsafe
as they explicitly rely onmethod_missing
which Sorbet can't deal with#7782