Skip to content
This repository has been archived by the owner. It is now read-only.

Track Bundler version in lockfile #3485

Closed
wants to merge 13 commits into from
Closed

Conversation

@stefanlance
Copy link
Contributor

@stefanlance stefanlance commented Mar 20, 2015

Let me know if anything should be changed!

lock_ver = ""
curr_ver = ""

if !lockfile.empty? and lockfile.match(/^ \[(.*)\]$/)

This comment has been minimized.

@indirect

indirect Mar 20, 2015
Member

Since the idiomatic !thing.empty? in ruby is thing.any?, I think it would probably make sense to switch to that.

This comment has been minimized.

@segiddins

segiddins Mar 20, 2015
Member

Except I believe thing.any? is linear on size?

def lock_version
lockfile = @lockfile_contents
lock_ver = ""
curr_ver = ""

This comment has been minimized.

@indirect

indirect Mar 20, 2015
Member

Unless I'm missing something, these variables can just be nil, and then your .empty? checks can simply be .nil? checks

@stefanlance stefanlance force-pushed the stefanlance:master branch from f7a02ef to 8f31d9e Mar 20, 2015
@stefanlance stefanlance force-pushed the stefanlance:master branch from 8f31d9e to d926226 Mar 20, 2015
end

ver
end

This comment has been minimized.

@indirect

indirect Mar 20, 2015
Member

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

This comment has been minimized.

@stefanlance

stefanlance Mar 20, 2015
Author Contributor

Yes, that part is a bit verbose. Thanks!

Stefan Lance added 2 commits Mar 20, 2015
Stefan Lance
Stefan Lance
def lock_version
lockfile = @lockfile_contents
lock_ver = nil
curr_ver = nil

This comment has been minimized.

@indirect

indirect Mar 20, 2015
Member

Oh, with the new succinct code, all three of these lines can be removed. :)

lock_ver = nil
curr_ver = nil

lock_ver = @lockfile_contents[/^ \[(.*)\]$/, 1] if lockfile

This comment has been minimized.

@indirect

indirect Mar 20, 2015
Member

and I lied, this needs to be @lockfile_contents before those three lines can be removed.

Stefan Lance added 3 commits Mar 20, 2015
Stefan Lance
Stefan Lance
@indirect
Copy link
Member

@indirect indirect commented Mar 22, 2015

Seems like this build is still failing, any chance for a fix?

Stefan Lance
def to_lock
out = ""

# Record the version of Bundler that was used to create the lockfile
out << "LOCKED WITH\n"

This comment has been minimized.

@segiddins

segiddins Mar 23, 2015
Member

BUNDLED WITH?


# If the lockfile's recorded version is older than the current version
# of Bundler, we warn the user to update Bundler.
if lockfile.match(/^ \[(.*)\]$/)

This comment has been minimized.

@segiddins

segiddins Mar 23, 2015
Member

any reason this doesn't use the state-based parsing?

if lockfile.match(/^ \[(.*)\]$/)
lock_ver = $1
curr_ver = Bundler::VERSION
prerelease_text = lock_ver.match(/pre/) ? " --pre" : ""

This comment has been minimized.

@segiddins

segiddins Mar 23, 2015
Member

this ought to parse the Gem::Version to see if it's a pre-release version

@indirect
Copy link
Member

@indirect indirect commented Apr 1, 2015

This looks great! Can you squash this pull request down so it's a single logical change? Then it's good to merge. 🚢

@stefanlance
Copy link
Contributor Author

@stefanlance stefanlance commented Apr 7, 2015

Closing since #3546 was merged.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.