-
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
Improve error handling for missing dependency versions for github actions #11144
Conversation
@@ -35,6 +35,9 @@ def parse | |||
dependency_set += workfile_file_dependencies(file) | |||
end | |||
|
|||
dependencies_without_version = dependency_set.dependencies.select { |dep| dep.version.nil? } | |||
raise UpdateNotPossible, dependencies_without_version.map(&:name) unless dependencies_without_version.empty? |
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.
Can we use a more descriptive error message?
UpdateNotPossible
is overloaded and opaque to our users.
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.
Tagging @kbukum1 since he has a list of some of our common errors that already have paved paths.
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 created a new error class to handle this specific case
c6dcd8e
to
36a9651
Compare
36a9651
to
a7f6a67
Compare
|
||
sig { returns(T::Array[String]) } | ||
attr_reader :dependencies | ||
|
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 for adding a new class; I think you'll have to make some changes on the API end too to get this error to sho up. @kbukum1 , can you share some sample past PRs?
a7f6a67
to
085771a
Compare
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.
LGTM
4ce06e6
to
56728db
Compare
56728db
to
ae52141
Compare
What are you trying to accomplish?
Improves error handling by raising a clear
UnresolvableVersionError
error when dependencies lack version constraints, rather than failing with a generic Sorbet error. This helps users understand exactly which dependencies are failing.Anything you want to highlight for special attention from reviewers?
Chose to fail fast during parsing rather than proceeding with dependencies that are not semver. This provides clearer feedback and prevents Sorbet from raising confusing type errors later in the update process.
How will you know you've accomplished your goal?
When the parse method correctly identifies dependencies without versions and raises an
UnresolvableVersionError
rather than a Sorbet issue message.Checklist