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

Add `bundle remove` #6513

Merged
merged 36 commits into from Jul 2, 2018

Conversation

Projects
None yet
5 participants
@agrim123
Member

agrim123 commented Apr 27, 2018

Features of the command implemented:

  • Multiple gems support
$ bundle remove rack rails
  • Remove any empty block that might occur after removing the gem or otherwise present

Things yet to implement:

  • Add rm alias. Optional
  • Add --install flag to remove gems from .bundle.
  • Handling multiple gems on the same line.
  • Handle gem spec
  • Handle eval_gemfile cases (one case left)

Closes #6506

@agrim123 agrim123 force-pushed the agrim123:agr-bundler-remove branch from c50ee88 to fa90e03 Apr 27, 2018

@agrim123 agrim123 changed the title from Add `bundler remove` to Add `bundle remove` Apr 27, 2018

@agrim123 agrim123 requested a review from segiddins Apr 27, 2018

@segiddins

This looks like a good start, but I'd like to see the test coverage expanded to cover:

  1. Nested groups
  2. Source/platform/install_if/env blocks
  3. What if the gem is specified in the gemfile, but in a way the remove code doesn't handle?
  4. What if a dependency is specified multiple times, both with and without different requirements / sources ?
  5. What if the gemfile has multiple gems on the same line?
  6. What happens if eval_gemfile is used and there are actually multiple files that define the single gemfile?
  7. What if the dependency comes from using the gemspec DSL method?
@@ -91,6 +91,25 @@ def gemspec(opts = nil)
end
end
def remove_gems(gems)

This comment has been minimized.

@segiddins

segiddins May 1, 2018

Member

does this need to go in the DSL, or could this be implemented in the Injector class that already handles adding gems?

This comment has been minimized.

@agrim123

agrim123 May 1, 2018

Member

In add command, the injector class appends to a gemfile, whereas in this case, we need to rewrite the whole gemfile. So we will need to put both the logic in Injector class.
A new method remove (that contains the logic of remove command) and will be created in Injector class same as inject and the private function remove_gems_from_gemfile also to private of Injector class? It will be called from remove class as

Injector.remove(@gems, {})

But because Injector class specifies new_deps as the instance variable that may also be confusing.
But because we need to write a lock file as well so we need to update the @dependencies in Dsl. So I think that function is good there. But

This comment has been minimized.

@segiddins

segiddins May 9, 2018

Member

this still needs to be moved out of the DSL

This comment has been minimized.

@agrim123

agrim123 May 9, 2018

Member

Moving this method to Injector under same name and takes builder as input and is a private method?

@removed_deps = builder.remove_gems(@gems)
# remove gems from lockfile
@definition = builder.to_definition(Bundler.default_lockfile, {})

This comment has been minimized.

@segiddins

segiddins May 1, 2018

Member

looks like this doesnt need to be an instance variable

This comment has been minimized.

@agrim123

agrim123 May 1, 2018

Member

Removing it.

private
def remove_gems_from_gemfile

This comment has been minimized.

@segiddins

segiddins May 1, 2018

Member

does this actually assert that all requested gems can be removed by the regular expressions?

This comment has been minimized.

@agrim123

agrim123 May 1, 2018

Member

Not yet. Just the basic ones. It does not handle the nested groups well. Working on that for now.

bundle "remove rack"
expect(gemfile).to_not match(/gem "rack"/)

This comment has been minimized.

@segiddins

segiddins May 1, 2018

Member

can you please change these tests to check for equality? That'll make reviewing much easier

This comment has been minimized.

@agrim123

agrim123 May 1, 2018

Member

I was thinking about writing a test something like this

expected_gemfile = "source \"file://#{gem_repo1}\"\n"
expect(gemfile).to eq(expected_gemfile)

for some test, directly matching the gemfile? Is it fine to match this way? Instead of regex of even expect(gemfile).to_not include("gem \"minitest\"")

gem "rack"
G
bundle "remove rack"

This comment has been minimized.

@segiddins

segiddins May 1, 2018

Member

when the commands are expected to succeed, please use bundle!

This comment has been minimized.

@agrim123

agrim123 May 1, 2018

Member

Sure. Just discovered the bang method.

# store patterns of all gems to be removed
patterns = []
@gems.each do |g|
patterns << /gem "#{g}"/

This comment has been minimized.

@segiddins

segiddins May 1, 2018

Member

what if the Gemfile uses single quotes?

This comment has been minimized.

@agrim123

agrim123 May 1, 2018

Member

Thinking of something better to remove them as per your points.
Maybe write a method that would remove gems from the array of dependencies and then write to gemfile.

@agrim123

This comment has been minimized.

Member

agrim123 commented May 1, 2018

@segiddins In accordance with your points:

  1. Nested groups

Trying something for this

  1. Source/platform/install_if/env blocks

Will we have to create separate functions for all these cases? And can they be nested also? And if they become empty then remove empty blocks also?

  1. What if the gem is specified in the gemfile, but in a way the remove code doesn't handle?

Apart from the case of single quotes, if regex matches whole line is removed.

  1. What if a dependency is specified multiple times, both with and without different requirements / sources ?

In current scenario, all instances of gem from gemfile would be removed. And I suppose that we cannot specify same gem more than once? Are we talking about this case:

group :one do
  gem "rack", "1.1"
end
group :two do
  gem "rack", "1.1"
end

Only case when versions are same. For this case we need to then add a flag to ask for group also and throw error if flag is not specified.

  1. What if the gemfile has multiple gems on the same line?

Have not encountered this case? Can you point me to a spec for this (for any other method) or an example?

  1. What happens if eval_gemfile is used and there are actually multiple files that define the single gemfile?

Does this means the case of various gemfiles (custom and default)? In that case we would need to ask user which gemfile to operate on (BUNDLE_GEMFILE) and by default use default configuration of user.

  1. What if the dependency comes from using the gemspec DSL method?

I do not know how this is implemented or used? Can you brief me on this?

@agrim123 agrim123 force-pushed the agrim123:agr-bundler-remove branch from 2189293 to 5f35ec7 May 8, 2018

@segiddins

This comment has been minimized.

Member

segiddins commented May 8, 2018

Will we have to create separate functions for all these cases? And can they be nested also? And if they become empty then remove empty blocks also?

yes to all of them. though it shouldn't be necessary to write separate functions, we can handle all of them at once and just keep a stack of the blocks we're in

Apart from the case of single quotes, if regex matches whole line is removed.

we'll need to make this more strict -- to start out with, it should probably only remove if the only other thing on the line is whitespace or a comment -- it would be bad for us to remove more than we're supposed to

In current scenario, all instances of gem from gemfile would be removed

that's fine, just need to add test for that

Have not encountered this case? Can you point me to a spec for this (for any other method) or an example?

gem 'a'; gem "b"

[gem "a", "gem "b"]

puts(gem("a"), gem("b"))

are all simple examples -- not saying they are frequent, but since this is just ruby, that's a valid use of the DSL

Does this means the case of various gemfiles (custom and default)? In that case we would need to ask user which gemfile to operate on (BUNDLE_GEMFILE) and by default use default configuration of user.

See the Definition#eval_gemfile method, which users can call to (conditional) include additional gem statements in their gemfile, we have tests for it somewhere

I do not know how this is implemented or used? Can you brief me on this?

See DSL#gemspec, or the tests inside spec/install/gemfile/gemspec_spec.rb

gem "rack"; gem "rails"
G
expected_gemfile = "source \"file://#{gem_repo1}\"\n\n" \

This comment has been minimized.

@segiddins

segiddins May 9, 2018

Member

please use heredoc strings for these, it makes reviewing much easier

This comment has been minimized.

@agrim123

agrim123 May 9, 2018

Member

Made a gemfile_should_be matcher to make it one liner

@@ -91,6 +91,25 @@ def gemspec(opts = nil)
end
end
def remove_gems(gems)

This comment has been minimized.

@segiddins

segiddins May 9, 2018

Member

this still needs to be moved out of the DSL

def remove_gems_from_gemfile(removed_deps, gemfile_path)
# store patterns of all gems to be removed
patterns = []

This comment has been minimized.

@segiddins

segiddins May 9, 2018

Member

pattern = /gem\s+['"]#{Regexp.union(removed_deps)}\1|gem\(['"]#{Regexp.union(removed_deps)}\2\)/ ?
This will match everything in a single regular expression

This comment has been minimized.

@agrim123

agrim123 May 9, 2018

Member

This throws an error

TypeError: no implicit conversion of Bundler::Dependency into String

Looking into it.

This comment has been minimized.

@agrim123

agrim123 May 10, 2018

Member

What are \1 and \2 for? I searched them and they stand for SOH and STX? Why exactly do we need them?

re = Regexp.union(patterns)
new_gemfile = []
IO.readlines(gemfile_path).each {|line| new_gemfile << line unless line.match(re) }

This comment has been minimized.

@segiddins

segiddins May 9, 2018

Member

new_gemfile = IO.readlines.select ?

This comment has been minimized.

@agrim123
end
end
blocks = ["group ", "source ", "env ", "install_if"]

This comment has been minimized.

@segiddins

segiddins May 9, 2018

Member

why do some of these have a trailing space but not install_if ?

This comment has been minimized.

@agrim123

agrim123 May 9, 2018

Member

Fixed

blocks = ["group ", "source ", "env ", "install_if"]
blocks.each {|block| remove_nested_blocks(new_gemfile, block) }
File.open(gemfile_path, "w") {|file| file.puts new_gemfile.join.chomp }

This comment has been minimized.

@segiddins

segiddins May 9, 2018

Member

this should use SharedHelpers.filesystem_access

This comment has been minimized.

@agrim123
# count number of nested groups
gemfile.each_with_index do |line, index|
nested_blocks += 1 if gemfile[index + 1] =~ /#{block_name}/ && line =~ /#{block_name}/

This comment has been minimized.

@segiddins

segiddins May 9, 2018

Member

include?(block_name) ?

This comment has been minimized.

@agrim123

agrim123 May 9, 2018

Member

Needed an edge case for the last element to use include. Done

gemfile.each_with_index do |line, index|
next if line !~ /#{block_name}/
if gemfile[index + 1] =~ /end/

This comment has been minimized.

@segiddins

segiddins May 9, 2018

Member

this should probably only match /^\s*end\s*$ ? otherwise we could accidentally do something like remove a line with a comment that contains the work end

# Remove gems from .bundle if install flag is specified
Installer.install(Bundler.root, Bundler.definition) if @options["install"]
removed_deps.each {|dep| Bundler.ui.confirm "#{dep.name}(#{dep.requirement}) was removed." }

This comment has been minimized.

@segiddins

segiddins May 9, 2018

Member

"#{SharedHelpers.pretty_dependency(dep)} was removed ?

@agrim123

This comment has been minimized.

Member

agrim123 commented May 10, 2018

@segiddins I think we should change the @new_deps instance variable name here to @deps. It creates confusion for the removed_deps part!

@agrim123 agrim123 force-pushed the agrim123:agr-bundler-remove branch 2 times, most recently from 7f8f93b to 4c4738f May 11, 2018

@agrim123 agrim123 force-pushed the agrim123:agr-bundler-remove branch from aafc3f9 to e14fd24 May 19, 2018

@agrim123 agrim123 force-pushed the agrim123:agr-bundler-remove branch from 06087cd to b8b2c70 May 27, 2018

@agrim123

This comment has been minimized.

Member

agrim123 commented May 31, 2018

@segiddins I revisited an old test case. Suppose user wants to remove mutiple gems from gemfile but some of them are not present, should we abort the operation altogether (and not remove any gems) or remove those which are present?
Something like this

context "when some gems are not present in the gemfile" do
  it "shows warning for those not present and success for those that can be removed" do
    gemfile <<-G
      source "file://#{gem_repo1}"

      gem "rails"
      gem "minitest"
      gem "rspec"
    G

    bundle! "remove rails rack minitest"
    expect(out).to include("rails (>= 0) was removed.")
    expect(out).to include("minitest (>= 0) was removed.")
    expect(out).to include("`rack` is not specified in Gemfile so not removed.")
    gemfile_should_be <<-G
     source "file://#{gem_repo1}"

     gem "rspec"
    G
  end
end
@segiddins

This comment has been minimized.

Member

segiddins commented Jun 4, 2018

Suppose user wants to remove mutiple gems from gemfile but some of them are not present, should we abort the operation altogether (and not remove any gems) or remove those which are present?

I think we should abort the operation, that way the command can be fixed & re-run

# evalutes a gemfile to remove the specified gem
# from it.
def evaluate_gemfile(gemfile_path)

This comment has been minimized.

@segiddins

segiddins Jun 4, 2018

Member

this should have remove in the name

Removes gems from the Gemfile. If a gem is not found, Bundler prints a error message and if gem could not be removed due to any reason Bundler warns the user.
D
method_option "install", :type => :boolean, :banner =>
"Removes gems from bundle"

This comment has been minimized.

@segiddins

segiddins Jun 4, 2018

Member

Runs 'bundle install' after removing the gems from the gemfile

if deleted_dep.nil?
Bundler.ui.warn "`#{gem_name}` is not specified in Gemfile so not removed."
return []

This comment has been minimized.

@segiddins

segiddins Jun 4, 2018

Member

should this be next instead of return so other dependencies can be removed? Or should it be a hard error?

This comment has been minimized.

@agrim123

agrim123 Jun 4, 2018

Member

Used next and displayed a warning.
Wanted to clarify one thing though. when we evaluate the main gemfile we get all the dependencies that are present? So we have a valid check and we can safely abort the operation on the first iteration when the gem is not found. So, maybe raising an error is a good choice. Reverting back.

This comment has been minimized.

@agrim123

agrim123 Jun 4, 2018

Member

However there is one case that is confusing though here
This will throw error when it evaluates Gemfile-other.

# @param [Pathname] gemfile_path The Gemfile from which to remove dependencies.
def remove_gems_from_gemfile(gems, gemfile_path)
# store patterns of all gems to be removed
patterns = /gem\s+(['"])#{Regexp.union(gems)}\1|gem\((['"])#{Regexp.union(gems)}\2\)/

This comment has been minimized.

@segiddins

segiddins Jun 4, 2018

Member

should be \s* in between gem and \(

@segiddins

This is looking really really good ! Great work @agrim123 !

@@ -166,6 +166,17 @@ def check
Check.new(options).run
end
desc "remove [GEMS]", "Removes the gem from gemfile"

This comment has been minimized.

@segiddins

segiddins Jun 15, 2018

Member

from the Gemfile

@@ -166,6 +166,17 @@ def check
Check.new(options).run
end
desc "remove [GEMS]", "Removes the gem from gemfile"
long_desc <<-D
Removes gems from the Gemfile. If a gem is not found, Bundler prints a error message and if gem could not be removed due to any reason Bundler warns the user.

This comment has been minimized.

@segiddins

segiddins Jun 15, 2018

Member
Removes the specified gems from the Gemfile in the current directory.
end
def run
# Raise error if no gems are specified

This comment has been minimized.

@segiddins

segiddins Jun 15, 2018

Member

comment is not necessary -- it's just repeating what the line of code below is saying.

in general, comments should explain the why or how rather that the what, since that's what the code does :)

builder = Dsl.new
builder.eval_gemfile(Bundler.default_gemfile)
definition = builder.to_definition(lockfile_path, {})

This comment has been minimized.

@segiddins

segiddins Jun 15, 2018

Member

all this can probably now be replaced with Bundler.definition.gemfiles.each

builder.eval_gemfile(gemfile_path)
# remove gems from dependencies
removed_deps = remove_gems_from_dependencies(builder, @deps)

This comment has been minimized.

@segiddins

segiddins Jun 15, 2018

Member

this causes the following output, which is not quite right:

# Gemfile
eval_gemfile 'Gemfile2'
gem 'ab'
# Gemfile2
gem("ab")

=>

Removing gems from Gemfile
ab could not be removed.
No gems were removed from the gemfile.
Removing gems from Gemfile2
ab (>= 0) was removed.

This comment has been minimized.

@agrim123

agrim123 Jun 15, 2018

Member

Looking into this.

This comment has been minimized.

@agrim123

agrim123 Jun 15, 2018

Member

How about if we add a method gemfile to Dependency that will hold the path to gemfile to which the gem belongs? In that way, we can easily filter out correct gems.
Or maybe just pass it into options when we create a new dependency.

This comment has been minimized.

@agrim123

agrim123 Jun 17, 2018

Member

Added the gemfile here

deleted_dep = builder.dependencies.find {|d| d.name == gem_name }
if deleted_dep.nil?
raise GemfileError, "`#{gem_name}` is not specified in Gemfile so could not be removed."

This comment has been minimized.

@segiddins

segiddins Jun 15, 2018

Member

should use the actual path in the error message, and so it could not be removed

nested_blocks -= 1
gemfile.each_with_index do |line, index|
next if line !~ /#{block_name}/

This comment has been minimized.

@segiddins

segiddins Jun 15, 2018

Member

next unless line.include?(block_name)

This comment has been minimized.

@agrim123

agrim123 Jun 15, 2018

Member

We also need an additional check !line.nil?

This comment has been minimized.

@segiddins

segiddins Jun 15, 2018

Member

we shouldn't need that, since the gemfile array should all be non-nil ?

end
# remove nil elements
gemfile.reject!(&:nil?)

This comment has been minimized.

@segiddins

segiddins Jun 15, 2018

Member

gemfile.compact!

unless extra_removed_gems.empty?
write_to_gemfile(gemfile_path, initial_gemfile.join)
raise InvalidOption, "Gems could not be removed."

This comment has been minimized.

@segiddins

segiddins Jun 15, 2018

Member

this should be a bit more detailed -- Gems could not be removed -- accidentally removed gems x, y, z or something like that?

This comment has been minimized.

@agrim123

agrim123 Jun 15, 2018

Member

How about Gems could not be removed. #{extra_removed_gems.join(", ")} will also be removed. Aborting."

This comment has been minimized.

@segiddins

segiddins Jun 15, 2018

Member

would also have been removed 👍

@@ -194,13 +194,15 @@ def ensure_same_dependencies(spec, old_deps, new_deps)
"\nEither installing with `--full-index` or running `bundle update #{spec.name}` should fix the problem."
end
def pretty_dependency(dep, print_source = false)
def pretty_dependency(dep, print_source = false, print_requirements = false)

This comment has been minimized.

@segiddins

segiddins Jun 15, 2018

Member

why the third parameter? I dont think we _ever want to print (>= 0)

This comment has been minimized.

@agrim123

agrim123 Jun 15, 2018

Member

Yeah. removed it 😄

@segiddins segiddins requested review from indirect and colby-swandale Jun 15, 2018

@agrim123 agrim123 force-pushed the agrim123:agr-bundler-remove branch from 848f667 to 431b00a Jun 15, 2018

@bundlerbot

This comment has been minimized.

Contributor

bundlerbot commented Jun 15, 2018

☔️ The latest upstream changes (presumably #6547) made this pull request unmergeable. Please resolve the merge conflicts.

@agrim123 agrim123 force-pushed the agrim123:agr-bundler-remove branch 2 times, most recently from c31e552 to ab428c1 Jun 15, 2018

@@ -166,6 +166,17 @@ def check
Check.new(options).run
end
desc "remove [GEMS] [OPTIONS]", "Removes the gem from the Gemfile"
long_desc <<-D
Removes the specified gems from the Gemfile in the current directory. If a gem is not found, Bundler prints a error message and if gem could not be removed due to any reason Bundler displays warning.

This comment has been minimized.

@colby-swandale

colby-swandale Jun 17, 2018

Member

in the current directory

This is not necessarily true, it should remove the gems from whichever Gemfile is configured as well.

This comment has been minimized.

@colby-swandale

colby-swandale Jun 17, 2018

Member

and if gem

the gem

This comment has been minimized.

@colby-swandale

colby-swandale Jun 17, 2018

Member

any reason Bundler displays warning

any reason, Bundler will display a warning

Removes the specified gems from the Gemfile in the current directory. If a gem is not found, Bundler prints a error message and if gem could not be removed due to any reason Bundler displays warning.
D
method_option "install", :type => :boolean, :banner =>
"Runs 'bundle install' after removing the gems from the gemfile"

This comment has been minimized.

@colby-swandale

colby-swandale Jun 17, 2018

Member

gemfile

Gemfile

# evalutes a gemfile to remove the specified gem
# from it.
def remove_deps(gemfile_path)
# get initial snap shot of the gemfile

This comment has been minimized.

@colby-swandale

colby-swandale Jun 17, 2018

Member

I find that comments like these don't really offer any useful information as they're only describing what the code is doing, which we can see already from the code. There is also the risk that they could fall out of date and end up being wrong the in the future.

I would prefer we remove these

removed_deps - errored_deps
end
def show_warning(message)

This comment has been minimized.

@colby-swandale

colby-swandale Jun 17, 2018

Member

Bundler::UI::Shell has a #warn method the will print text in yellow already.

This comment has been minimized.

@agrim123

agrim123 Jun 17, 2018

Member

It deduplicates the message. Sometimes we need to show the same message for mutiple gemfiles.

## DESCRIPTION
Removes a set of specified gems from the Gemfile or if needed from the bundle itself.

This comment has been minimized.

@colby-swandale

colby-swandale Jun 17, 2018

Member

if needed from the bundle itself

This won't make a huge amount of sense to a lot of people. I would instead add a paragraph that explains the --install option.

Removes a set of specified gems from the Gemfile or if needed from the bundle itself.
If any gem specified to be removed does not exist the in Gemfile, Bundler prints error message.
If gem could not be removed due to any reason Bundler warns the user.

This comment has been minimized.

@colby-swandale

colby-swandale Jun 17, 2018

Member

If gem

the Gem

This comment has been minimized.

@colby-swandale

colby-swandale Jun 17, 2018

Member

reason Bundler warns the user

reason, Bundler warns the user

@colby-swandale

This comment has been minimized.

Member

colby-swandale commented Jun 17, 2018

Does this feature make the distinction between a gem not existing in the Gemfile and not being able to find the gem in the Gemfile? Seeing the Gemfile can be written in pretty much any way using Ruby. There will be a cases where i have a gem in the Gemfile but is not specified using gem 'mygem'

@agrim123

This comment has been minimized.

Member

agrim123 commented Jun 17, 2018

We are only considering the case of a standard gemfile. We can't know apart from standard how things are in Gemfile so we remove based on only standard procedure.

@colby-swandale

This comment has been minimized.

Member

colby-swandale commented Jun 17, 2018

Apologies, i wasn't clear on this. It's probably good in terms of error handling to print an error to the user when they want to remove a gem in their Gemfile, but we couldn't find & remove the declaration in the Gemfile.

edit: nevermind, i can see this already being handled.

@agrim123 agrim123 force-pushed the agrim123:agr-bundler-remove branch 2 times, most recently from 876b530 to b082439 Jun 19, 2018

@agrim123 agrim123 force-pushed the agrim123:agr-bundler-remove branch from b082439 to df258d9 Jun 28, 2018

@indirect

I like this, thanks @agrim123! I've added some suggestions for the wording, and I'm happy to merge this as soon as those changes are made.

@@ -0,0 +1,25 @@
bundle-remove(1) -- Removes gem specified in Gemfile

This comment has been minimized.

@indirect

indirect Jul 1, 2018

Member

Can we update this to: bundle-remove(1) -- Removes gems from the Gemfile?

## SYNOPSIS
`bundle remove [GEMS] [--install]`

This comment has been minimized.

@indirect

indirect Jul 1, 2018

Member

Can we update this to:
`bundle remove [GEM [GEM ...]] [--install]`

Removes a set of specified gems from the Gemfile or if needed runs `bundle install` and removes them from bundle as well.
If any gem specified to be removed does not exist the in Gemfile, Bundler prints error message.
If the gem could not be removed due to any reason, Bundler warns the user.

This comment has been minimized.

@indirect

indirect Jul 1, 2018

Member

Can we update this to:

Removes the given gems from the Gemfile while ensuring that the resulting Gemfile is still valid. If a gem cannot be removed, a warning is printed. If a gem is already absent from the Gemfile, and error is raised.

## OPTIONS
* `--install`:
Runs 'bundle install' after removing the gems from the gemfile. This option sync the lockfile.

This comment has been minimized.

@indirect

indirect Jul 1, 2018

Member

Can we change this to:

Runs bundle install after the given gems have been removed from the Gemfile, which ensures that both the lockfile and the installed gems on disk are also updated to remove the given gem(s).

@@ -166,6 +166,17 @@ def check
Check.new(options).run
end
desc "remove [GEMS] [OPTIONS]", "Removes the gem from the Gemfile"

This comment has been minimized.

@indirect

indirect Jul 1, 2018

Member

Can we update this to:

desc "remove [GEM [GEM ...]]", "Removes gems from the Gemfile"

@@ -166,6 +166,17 @@ def check
Check.new(options).run
end
desc "remove [GEMS] [OPTIONS]", "Removes the gem from the Gemfile"
long_desc <<-D
Removes the specified gems from the Gemfile. If the gem is not found, Bundler prints a error message and if gem could not be removed due to any reason Bundler will display a warning.

This comment has been minimized.

@indirect

indirect Jul 1, 2018

Member

Can we update this to:

Removes the given gems from the Gemfile while ensuring that the resulting Gemfile is still valid.

@segiddins

This comment has been minimized.

Member

segiddins commented Jul 2, 2018

@indirect your feedback should be addressed now!

@indirect

This comment has been minimized.

Member

indirect commented Jul 2, 2018

Perfect 👍🏻

@bundlerbot r+

@bundlerbot

This comment has been minimized.

Contributor

bundlerbot commented Jul 2, 2018

📌 Commit abaa283 has been approved by indirect

@bundlerbot

This comment has been minimized.

Contributor

bundlerbot commented Jul 2, 2018

⌛️ Testing commit abaa283 with merge 431a3f7...

bundlerbot added a commit that referenced this pull request Jul 2, 2018

Auto merge of #6513 - agrim123:agr-bundler-remove, r=indirect
Add `bundle remove`

Features of the command implemented:
- Multiple gems support
```bash
$ bundle remove rack rails
```
- Remove any empty block that might occur after removing the gem or otherwise present

Things yet to implement:
- Add `rm` alias. _Optional_
- [x] Add `--install` flag to remove gems from `.bundle`.
- [x] Handling multiple gems on the same line.
- [x] Handle gem spec
- [x] Handle eval_gemfile cases ([one](#6513 (comment)) case left)

Closes #6506
@bundlerbot

This comment has been minimized.

Contributor

bundlerbot commented Jul 2, 2018

☀️ Test successful - status-travis
Approved by: indirect
Pushing 431a3f7 to master...

@bundlerbot bundlerbot merged commit abaa283 into bundler:master Jul 2, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@agrim123 agrim123 deleted the agrim123:agr-bundler-remove branch Jul 2, 2018

@colby-swandale colby-swandale added this to the 1.17.0 milestone Oct 2, 2018

colby-swandale added a commit that referenced this pull request Oct 9, 2018

Auto merge of #6513 - agrim123:agr-bundler-remove, r=indirect
Add `bundle remove`

Features of the command implemented:
- Multiple gems support
```bash
$ bundle remove rack rails
```
- Remove any empty block that might occur after removing the gem or otherwise present

Things yet to implement:
- Add `rm` alias. _Optional_
- [x] Add `--install` flag to remove gems from `.bundle`.
- [x] Handling multiple gems on the same line.
- [x] Handle gem spec
- [x] Handle eval_gemfile cases ([one](#6513 (comment)) case left)

Closes #6506

(cherry picked from commit 431a3f7)

colby-swandale added a commit that referenced this pull request Oct 25, 2018

Merge branch '1-17-stable'
* 1-17-stable: (38 commits)
  Version 1.17.0 with changelog
  Merge #6754
  Version 1.17.0.pre.2 with changelog
  fix failing bundle remove specs
  Still document the `--force` option
  scope specs testing bundler 2 deprecations to bundler 1 only
  Merge #6718
  Merge #6707
  Merge #6702
  Merge #6316
  Auto merge of #6447 - agrim123:agr-update-error-message, r=segiddins
  Auto merge of #6513 - agrim123:agr-bundler-remove, r=indirect
  Auto merge of #6318 - jhawthorn:fix_incorrect_test_in_requires_sudo, r=segiddins
  Auto merge of #6450 - bundler:segiddins/bundle-binstubs-all, r=colby-swandale
  Auto merge of #6024 - gwerbin:custom-user-bundle-dir, r=colby-swandale
  Version 1.17.0.pre.1 with changelog
  Auto merge of #5964 - bundler:colby/deprecate-viz-command, r=segiddins
  Auto merge of #5986 - bundler:seg-jobs-count, r=indirect
  Auto merge of #5995 - bundler:seg-gvp-major, r=indirect
  Auto merge of #5803 - igorbozato:path_relative_to_pwd, r=indirect
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment