Allow string keys with gemrc #7543
Conversation
lib/rubygems/config_file.rb
Outdated
| @disable_default_gem_server = @hash[:disable_default_gem_server] if @hash.key? :disable_default_gem_server | ||
| @sources = @hash[:sources] if @hash.key? :sources | ||
| @update_sources = @hash[:update_sources] if @hash.key? :update_sources | ||
| # concurrent_downloads |
There was a problem hiding this comment.
Is the above comment intentional?
There was a problem hiding this comment.
Yes. I'm not sure why concurrent_download is not handled this place. We should fix that for Gem.configuration.concurrent_downloads.
But It's not this PR scope.
There was a problem hiding this comment.
So basically this comment is acting as a TODO note? If that's the case, I'd make that a bit more clear like TODO: Review why concurrent_downloads is not handled here.
lib/rubygems/config_file.rb
Outdated
| @@ -210,22 +210,32 @@ def initialize(args) | |||
| @hash = @hash.merge environment_config | |||
| end | |||
|
|
|||
| @hash.transform_keys! do |k| | |||
| if %w[backtrace bulk_threshold verbose update_sources cert_expiration_length_days | |||
| install_extension_in_lib ipv4_fallback_enabled].include?(k) | |||
There was a problem hiding this comment.
How did you choose the list of configuration keys? I see there are a few more than this.
There was a problem hiding this comment.
Make sense. It's not strong reason.
I update that to deny list.
lib/rubygems/config_file.rb
Outdated
| @@ -210,22 +210,32 @@ def initialize(args) | |||
| @hash = @hash.merge environment_config | |||
| end | |||
|
|
|||
| @hash.transform_keys! do |k| | |||
| # These keys didn't work as symbols | |||
| if %w[gem install gemhome gempath].include?(k) | |||
There was a problem hiding this comment.
From the comment in lines 15-16 in this file:
gemrc is a YAML file that uses strings to match gem command arguments and
symbols to match RubyGems options.
My understanding is that right now, confusingly, you can specify any command name (or gem for all commands), and that's specified as string. For RubyGems options, they are specified as keys.
So, I think we should:
- Include all commands here, not just
install, otherwise configurations for other commands are going to be symbolized and possible break? - Remove or reword the comment above to no longer document this inconsistency.
Makes sense?
There was a problem hiding this comment.
I changed my proposal. I only convert Gem::ConfigFile accessors. I don't want to work this simple issue to Gem.configuration entirely.
Allow string keys with gemrc (cherry picked from commit bdb1468)
What was the end-user or developer problem that led to this PR?
I sometimes confused why it's not working correctly with the following
gemrc.The correct case is
:verbose:for that key. It's not useful for users.What is your fix for the problem, implemented in this PR?
I convert them to symbol keys if specified at gemrc.
Make sure the following tasks are checked