Skip to content
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

Prevent remove command from deleting gemfile lines that are comments #4045

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 9 additions & 2 deletions bundler/lib/bundler/injector.rb
Expand Up @@ -179,11 +179,11 @@ def remove_gems_from_dependencies(builder, gems, gemfile_path)
# @param [Pathname] gemfile_path The Gemfile from which to remove dependencies.
def remove_gems_from_gemfile(gems, gemfile_path)
patterns = /gem\s+(['"])#{Regexp.union(gems)}\1|gem\s*\((['"])#{Regexp.union(gems)}\2\)/

new_gemfile = []
multiline_removal = false
IO.readlines(gemfile_path).each do |line|
if line.match(patterns)
match_data = line.match(patterns)
if match_data && is_not_within_comment?(line, match_data)
multiline_removal = line.rstrip.end_with?(",")
# skip lines which match the regex
next
Expand All @@ -207,6 +207,13 @@ def remove_gems_from_gemfile(gems, gemfile_path)
new_gemfile.join.chomp
end

# @param [String] line Individual line of gemfile content.
# @param [MatchData] match_data Data about Regex match.
def is_not_within_comment?(line, match_data)
match_start_index = match_data.offset(0).first
!line[0..match_start_index].include?("#")
end

# @param [Array] gemfile Array of gemfile contents.
# @param [String] block_name Name of block name to look for.
def remove_nested_blocks(gemfile, block_name)
Expand Down
84 changes: 82 additions & 2 deletions bundler/spec/commands/remove_spec.rb
Expand Up @@ -83,7 +83,7 @@
end
end

describe "remove mutiple gems from gemfile" do
describe "remove multiple gems from gemfile" do
context "when all gems are present in gemfile" do
it "shows success fir all removed gems" do
gemfile <<-G
Expand Down Expand Up @@ -210,7 +210,7 @@
end
end

context "when the gem belongs to mutiple groups" do
context "when the gem belongs to multiple groups" do
it "removes the groups" do
gemfile <<-G
source "#{file_uri_for(gem_repo1)}"
Expand Down Expand Up @@ -616,4 +616,84 @@
expect(out).to include("foo could not be removed.")
end
end

describe "with comments that mention gems" do
context "when comment is a separate line comment" do
it "does not remove the line comment" do
gemfile <<-G
source "#{file_uri_for(gem_repo1)}"

# gem "rack" might be used in the future
gem "rack"
G

bundle "remove rack"

expect(out).to include("rack was removed.")
gemfile_should_be <<-G
source "#{file_uri_for(gem_repo1)}"

# gem "rack" might be used in the future
G
end
end

context "when gem specified for removal has an inline comment" do
deivid-rodriguez marked this conversation as resolved.
Show resolved Hide resolved
it "removes the inline comment" do
gemfile <<-G
source "#{file_uri_for(gem_repo1)}"

gem "rack" # this can be removed
G

bundle "remove rack"

expect(out).to include("rack was removed.")
gemfile_should_be <<-G
source "#{file_uri_for(gem_repo1)}"
G
end
end

context "when gem specified for removal is mentioned in other gem's comment" do
it "does not remove other gem" do
gemfile <<-G
source "#{file_uri_for(gem_repo1)}"
gem "puma" # implements interface provided by gem "rack"

gem "rack"
G

bundle "remove rack"

expect(out).to_not include("puma was removed.")
expect(out).to include("rack was removed.")
gemfile_should_be <<-G
source "#{file_uri_for(gem_repo1)}"
gem "puma" # implements interface provided by gem "rack"
G
end
end

context "when gem specified for removal has a comment that mentions other gem" do
it "does not remove other gem" do
gemfile <<-G
source "#{file_uri_for(gem_repo1)}"
gem "puma" # implements interface provided by gem "rack"

gem "rack"
G

bundle "remove puma"

expect(out).to include("puma was removed.")
expect(out).to_not include("rack was removed.")
gemfile_should_be <<-G
source "#{file_uri_for(gem_repo1)}"

gem "rack"
G
end
end
end
end