Skip to content
This repository was archived by the owner on Apr 14, 2021. It is now read-only.

[Feature] bundle change#6597

Closed
agrim123 wants to merge 23 commits intorubygems:masterfrom
agrim123:agr-bundle-change
Closed

[Feature] bundle change#6597
agrim123 wants to merge 23 commits intorubygems:masterfrom
agrim123:agr-bundle-change

Conversation

@agrim123
Copy link
Copy Markdown
Contributor

@agrim123 agrim123 commented Jun 22, 2018

Currently, this command uses remove and add internally (reason for so many commits, though only last five, are related to this command).

Closes #6581

@agrim123 agrim123 force-pushed the agr-bundle-change branch 3 times, most recently from d4b7f20 to 4831906 Compare June 26, 2018 09:45
@segiddins
Copy link
Copy Markdown
Contributor

Can you change the target branch of this to agr-bundler-remove so it's possible to leave comments on just these changes?

@agrim123
Copy link
Copy Markdown
Contributor Author

agr-bundler-remove is on my remote, so I think we will have to wait till it gets merged.

@agrim123 agrim123 force-pushed the agr-bundle-change branch 3 times, most recently from 668acd3 to 831214f Compare July 2, 2018 05:32
@agrim123
Copy link
Copy Markdown
Contributor Author

agrim123 commented Jul 2, 2018

@segiddins Now this is reviewable 😄

require "bundler/cli/add"
CLI::Add.new(@pass_options, [@gem_name]).run
rescue StandardError => e
Bundler.ui.error e
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the error be re-raised instead, after restoring the initial gemfile?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is shown as error do we need to raise an exception?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we should let the FriendlyErrors module handle showing the error to the user, and ensure we exit with the proper exit code


private

def set_version_options
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment explaining what this method is trying to do ?


# @param [Pathname] gemfile_path The Gemfile from which to remove dependencies.
# @param [String] contents Content to written to Gemfile.
def write_to_gemfile(gemfile_path, contents)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably just be write_file

it "throws error" do
bundle "change rack"

expect(out).to include("Please supply atleast one option to change.")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at least


set_version_options

@pass_options["source"] = @options[:source] if @options[:source]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this also handle path/git ?

Copy link
Copy Markdown
Contributor Author

@agrim123 agrim123 Jul 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should they be editable? We would have to do something to persist them 🤔 as add does not take these flags

Copy link
Copy Markdown
Contributor Author

@agrim123 agrim123 Jul 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also @segiddins , these are valid keys we also need to persist them! I think we may have to change our approach? Maybe we can use this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah we definitely need to persist all keys

@agrim123 agrim123 force-pushed the agr-bundle-change branch 2 times, most recently from a612020 to d6faf03 Compare July 3, 2018 08:41
"Adds gem to the Gemfile but does not install it"
method_option "optimistic", :type => :boolean, :banner => "Adds optimistic declaration of version to gem"
method_option "strict", :type => :boolean, :banner => "Adds strict declaration of version to gem"
method_option "pessimistic", :type => :boolean, :banner => "Adds pessimistic declaration of version to gem"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this option needs to be tested

Provide flexibilty of editing gemfile from command line by providing option to change gem properties.
D
method_option "version", :aliases => "-v", :type => :string
method_option "group", :aliases => "-g", :type => :string
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's not add this aliases for now

CLI::Add.new(@pass_options, [@gem_name]).run
rescue StandardError => e
SharedHelpers.write_file(Bundler.default_gemfile, initial_gemfile)
FriendlyErrors.log_error(e)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instead of this, we should just raise

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After rescue then again we should raise error?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes -- the rescue here is only to restore the initial gemfile


raise InvalidOption, "`#{@gem_name}` could not be found in the Gemfile." unless @dep

@pass_options = {}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe call this add_options, and instead of making it an ivar, pass it into set_group_options / set_version_options


definition = Bundler.definition

@dep = definition.dependencies.find {|d| d.name == @gem_name }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as below, shouldn't be an ivar

## DESCRIPTION
Provide flexibilty of editing gemfile from command line by providing option to change gem properties.

Example:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this example should show the before & after gemfiles

gem "rspec"
end

gem "rack", "~> 1.0", :group => [:dev1]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should either be :group => :dev1 or :groups => [:dev1]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handled here

source "file://#{gem_repo1}"

gem "rack", "~> 1.0", :group => [:dev]
gem "weakling", ">= 0.0.1"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a test that changes both of these 2 gems into the test group

it "changes version of the gem" do
bundle! "change rack --version 0.9.1"

expect(bundled_app("Gemfile").read).to include("gem \"rack\", \"~> 0.9.1\", :group => [:dev]")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use single quotes to avoid having to escape the double quotes

it "throws error" do
bundle "change rake --group dev1"

expect(out).to include("`rake` could not be found in the Gemfile.")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please test that failure does not change the gemfile

expect { bundle ... }.not_to change { gemfile }

Copy link
Copy Markdown
Contributor Author

@agrim123 agrim123 Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No operation is done on gemfile until this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right but we should test that that's the case :)

@segiddins
Copy link
Copy Markdown
Contributor

This looks like a great 1st implementation of the bundle change feature to me! Well done @agrim123 !

@colby-swandale @deivid-rodriguez review would be appreciated 🙏


raise InvalidOption, "`#{@gem_name}` could not be found in the Gemfile." unless dep

check_for_unsupported_options(dep)
Copy link
Copy Markdown
Contributor Author

@agrim123 agrim123 Jul 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in process of writing test for this function. Still to figure out how to skip before hook for one test.

@agrim123 agrim123 force-pushed the agr-bundle-change branch from 5c38781 to 4660a3e Compare July 27, 2018 12:02
@colby-swandale colby-swandale modified the milestone: 1.17.0 Oct 9, 2018
@agrim123
Copy link
Copy Markdown
Contributor Author

agrim123 commented Jan 4, 2019

@segiddins @colby-swandale The milestone seems to be have passed 😅

@fatkodima
Copy link
Copy Markdown
Contributor

Is it just me, but I don't see any benefits of adding and using this? It is much easier to make changes directly in Gemfile and run bundle install/bundle update gemname, than remembering that there is yet another change command and how to use it, what are the arguments.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add bundle change

5 participants