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

Add subcommands to config #4600

Closed
indirect opened this issue May 22, 2016 · 11 comments · Fixed by #5981
Closed

Add subcommands to config #4600

indirect opened this issue May 22, 2016 · 11 comments · Fixed by #5981

Comments

@indirect
Copy link
Member

Today, it's confusing that config lets you pass the flags local, global, and delete in any combination.
It would be an improvement to the interface of config if Bundler 2 supported a slightly different set of commands, e.g.:

# Setting
$ bundle config set foo bar
# Unsetting
$ bundle config unset foo bar
# Local-only
$ bundle config set foo bar --local
# Global-only
$ bundle config set foo bar --global
@indirect indirect added this to the 2.0 -- Breaking Changes milestone May 22, 2016
@denniss
Copy link
Contributor

denniss commented Mar 7, 2017

@indirect may I give this one a shot?

@indirect
Copy link
Member Author

indirect commented Mar 7, 2017

yes, definitely! sounds awesome :)

@denniss
Copy link
Contributor

denniss commented Mar 7, 2017

Is it necessary to have an RFC for this first or can I just start working based on the example you've given?

@indirect
Copy link
Member Author

indirect commented Mar 7, 2017

I think starting an implementation is a good way to figure out if there are issues that we need to discuss. :)

@denniss
Copy link
Contributor

denniss commented Mar 8, 2017

@indirect I am using existing config_spec to test my implementation of this feature. While looking through the existing specs, I found this one to be somewhat confusing: https://github.com/bundler/bundler/blob/master/spec/commands/config_spec.rb#L217

Specifically, shouldn't the line:

run "puts Bundler.settings.send(:global_config_file).read"

be

run "puts Bundler.settings.send(:local_config_file).read"

Since the previous command is calling bundle config foo '1'.

@indirect
Copy link
Member Author

indirect commented Mar 8, 2017

@denniss yup, that looks like a mistake. No idea how it's passing. 😮

@denniss
Copy link
Contributor

denniss commented Mar 8, 2017

I think that actually might be the expected behavior. Although, I find it kind of confusing. From bundle help config

Executing bundle config will set that configuration to the value specified for all bundles executed as the current user. The configuration will be stored in ~/.bundle/config. If name already is set, name will be overridden and user will be warned.

I'd like to propose a couple change to the behavior of the config command.

  • I feel like global configuration should only be changed when --global is specified.
  • Setting a value where there are spaces involve should require quotes so that there is clear separation between which part is the value and which part is the command/options

E.g.
bundle set config build.mysql --with-mysql-config=/usr/local/mysql/bin/mysql_config
Should be
bundle set config build.mysql '--with-mysql-config=/usr/local/mysql/bin/mysql_config'

May be @segiddins can chime in on this?

@segiddins
Copy link
Member

Those are both breaking changes, which we wouldn't be able to ship in Bundler 1.x

@indirect
Copy link
Member Author

indirect commented Mar 8, 2017

@segiddins yes, this entire PR is already in 2.0 territory. But I'm comfortable with starting work on that now, I think.

@indirect
Copy link
Member Author

indirect commented Mar 8, 2017

@denniss I like both of those changes, I think 👍

@segiddins
Copy link
Member

In that case, I love the idea :)

bundlerbot added a commit that referenced this issue Sep 5, 2017
[2.0] Implement config subcommands

### What was the end-user problem that led to this PR?

The problem was the current `bundle config` mega-command is hacky and confusing.

### What was your diagnosis of the problem?

My diagnosis was we should add subcommands for `list`, `get`, `set`, and `unset` to make the CLI clearer.

Closes #4600.

### What is your fix for the problem, implemented in this PR?

My fix implements those subcommands while preserving the current bare `bundle config` command.

### Why did you choose this fix out of the possible options?

I chose this fix because, as opposed to #5507, we can keep the existing command in for a little while to help ease the transition.

@denniss how do you feel about this compared to your PR?
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants