-
Notifications
You must be signed in to change notification settings - Fork 1.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
adding sorbet types for elm file parser #11440
base: main
Are you sure you want to change the base?
Conversation
c111a54
to
d55529e
Compare
|
||
sig { override.returns(T::Array[Dependabot::Dependency]) } |
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 don't think you actually need override here
@@ -149,7 +162,8 @@ def build_elm_json_dependency(name:, group:, requirement:, direct:) | |||
|
|||
sig { returns(String) } | |||
def repo_type | |||
parsed_elm_json.fetch("type") | |||
type = parsed_elm_json.fetch("type") | |||
T.must(type.is_a?(String) ? type : 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.
Not sure what this is doing but I think the result is nilable so I don't think the signature should be sig { returns(String) }
@@ -167,12 +186,17 @@ def version_for(version_requirement) | |||
req.requirements.first.last | |||
end | |||
|
|||
sig { returns(T::Hash[String, T.any(String, T::Hash[String, T.any(String, T::Hash[String, 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.
I would monitor this after deploying. It's too precise and it wouldn't surprise me if it starts erroring.
What are you trying to accomplish?
Add sorbet types to:
elm/lib/dependabot/elm/file_parser.rb
How will you know you've accomplished your goal?
All tests should continue to pass as before and there shouldn't be any new sorbet runtime errors in Sentry
Checklist