-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix required_ruby_version
issue when using Gem::Requirement
#9037
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -141,9 +141,17 @@ def find_version | |
return versions.compact.min | ||
end | ||
|
||
return gem_requirement_version(version) if version.send_type? | ||
|
||
version_from_str(version.str_content) | ||
end | ||
|
||
def gem_requirement_version(version) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this is still extracted from the gemspec it should probably be part of that existing method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New arrangement based on #9037 (comment), let me know your thoughts @bbatsov . |
||
gem_requirement = version.children.last | ||
versions = gem_requirement.children.map { |v| version_from_str(v) } | ||
versions.compact.min | ||
end | ||
|
||
def gemspec_filename | ||
@gemspec_filename ||= begin | ||
basename = Pathname.new(@config.base_dir_for_path_parameters).basename.to_s | ||
|
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.
Probably we should do a more precise node match here, as there's a slight chance some other code might be used here.
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 @bbatsov!
There is now a more precise matching pattern.
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.
Looks good! Maybe you can merge it with
if version.array_type?
, also capturing the versions, and you can reuse the samemap
,compact
,min
. Maybe not though, just a thoughtThere 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'll look into it @marcandre!
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.
{ (array $str+) ... }
can help, and there's https://nodepattern.herokuapp.com/ to help test things.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.
So now the
map
,compact
andmin
are reused and I definitely think it looks more DRY.I managed to merge it by creating a
version_from_array
method but didn't do any newNodePattern
.