-
Notifications
You must be signed in to change notification settings - Fork 41
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
Filter versions while testing #286
Conversation
cf6565b
to
04ae129
Compare
538b616
to
0ad1695
Compare
f9e8b05
to
f12cd6c
Compare
f12cd6c
to
f0e3028
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.
Thanks!
|
||
private | ||
|
||
sig { params(gem_name: String, annotations_file: String).returns(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 wonder if this should go in the parent class?
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.
Runtime doesn't need it at the moment but yeah it's not specific to static. I'll move it.
|
||
sig { params(gem_name: String, annotations_file: String).returns(String) } | ||
def filter_versions_from_annotation(gem_name, annotations_file) | ||
gem_version = ::Gem::Version.new(gem_version_from_gemfile_lock(gem_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.
Maybe the upstream gem_version_from_gemfile_lock
should return a Version
instance rather than a 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.
Yeah that aligns better with the name. I can add a to_s
to the usages in Spoom that expects a string. I'll unblock rbi-central
with the current change and bump spoom in another PR after it's implemented.
f0e3028
to
b5f3391
Compare
Preliminary support for testing annotations with versions.
In a future improvement we should test all versions and not just one used in CI which should be the latest version: #287