-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Conversation
lock_ver = "" | ||
curr_ver = "" | ||
|
||
if !lockfile.empty? and lockfile.match(/^ \[(.*)\]$/) |
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.
Since the idiomatic !thing.empty?
in ruby is thing.any?
, I think it would probably make sense to switch to that.
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.
Except I believe thing.any?
is linear on size?
end | ||
|
||
ver | ||
end |
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 I kept mulling this over, and I think I see some ways to cut down on the amount of code that exists without making it (much?) more obscure:
lock_ver = @lockfile_contents[/^ \[(.*)\]$/, 1] if lockfile
if lock_ver && Gem::Version.new(lock_ver) < Gem::Version.new(Bundler::VERSION)
new_ver = Bundler::VERSION
end
new_ver || lock_ver || Bundler::VERSION
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.
Yes, that part is a bit verbose. Thanks!
def lock_version | ||
lockfile = @lockfile_contents | ||
lock_ver = nil | ||
curr_ver = 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.
Oh, with the new succinct code, all three of these lines can be removed. :)
Seems like this build is still failing, any chance for a fix? |
def to_lock | ||
out = "" | ||
|
||
# Record the version of Bundler that was used to create the lockfile | ||
out << "LOCKED WITH\n" |
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.
BUNDLED WITH
?
This looks great! Can you squash this pull request down so it's a single logical change? Then it's good to merge. 🚢 |
Closing since #3546 was merged. |
Let me know if anything should be changed!