Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

This commit fixes issue #8628 #8633

Merged
merged 1 commit into from

4 participants

@mykola-kyryk

Allow environment name to start with a substring of the default
environment names.
For example: tes, pro, prod, dev, devel, etc.

@senny
Owner

@mykola-kyryk could you add a test-case?

railties/lib/rails/commands/console.rb
((10 lines not shown))
end
options
end
+
+ private
+ def available_environments

No need to indent.

@rafaelfranca Owner

No it is right. But private has the same indentation of the previous method. Everything bellow it is indented.

Ok. Thanks. Will fix that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@oscardelben oscardelben commented on the diff
railties/lib/rails/commands/console.rb
@@ -24,11 +24,20 @@ def parse_arguments(arguments)
if arguments.first && arguments.first[0] != '-'
env = arguments.first
- options[:environment] = %w(production development test).detect {|e| e =~ /^#{env}/} || env
+ if available_environments.include? env
+ options[:environment] = env
+ else
+ options[:environment] = %w(production development test).detect {|e| e =~ /^#{env}/} || env

Why not use available_environments here as well? Also, I think a single check would be enough, as detect would work in all cases.

#available_environments method return all environments (including any custom environments), where detect iterates over three default ones.
Previous code was great to run 'rails console p' and have 'production' environment loaded into console.
But it would work incorrectly if you have custom environment called 'pro' and try to run 'rails console pro', it would load production environment, not the custom 'pro' one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mykola-kyryk

Test added.

@senny senny commented on the diff
railties/test/commands/console_test.rb
@@ -111,6 +111,12 @@ def test_rails_env_is_development_when_argument_is_d
assert_match(/\sdevelopment\s/, output)
end
+ def test_rails_env_is_dev_when_argument_is_dev_and_dev_env_is_present
+ Rails::Console.stubs(:available_environments).returns(['dev'])
@senny Owner
senny added a note

I see the purpose of this stub but stubbing private methods from the instance under test gives me a bad feeling. I'm not completely sure what the stubbing/mocking policy from rails is but this does not feel right to me.

@rafaelfranca can you elaborate?

'available_environments' method is a private class method. It works with file system, so I stub it in the test.

Also, can anyone point me to a guide on rails tests?

@rafaelfranca Owner

For me is fine to stub private methods if the method is doing something expensive or depends of the environment.

We don't have any policy, we follow our :heart:. My :heart: is saying that for this case it is fine.

I'll take a look later on this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca
Owner

I think we will have to fix the same thing on dbconsole too

@rafaelfranca
Owner
ack "production development test"
lib/rails/commands/console.rb
27:          options[:environment] = %w(production development test).detect {|e| e =~ /^#{env}/} || env

lib/rails/commands/dbconsole.rb
139:        options[:environment] = %w(production development test).detect {|e| e =~ /^#{env}/} || env
@mykola-kyryk

Will do )

@mykola-kyryk
@rafaelfranca
Owner

It needs a rebase, also could you squash your commits?

Mykola Kyryk This commit fixes issue #8628
Allow environment name to start with a substring of the default
environment names.
For example: tes, pro, prod, dev, devel, etc.

Fixing identation.

Adding test for Rails::Console.parse_arguments method.

Fix issue 8628 for Rails::DBConsole.
4fa6088
@rafaelfranca rafaelfranca merged commit fab7c53 into from
@rafaelfranca
Owner

Thanks

@mykola-kyryk

Rafael, you are the best. Thanks a lot for your help and guidance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 4, 2013
  1. This commit fixes issue #8628

    Mykola Kyryk authored
    Allow environment name to start with a substring of the default
    environment names.
    For example: tes, pro, prod, dev, devel, etc.
    
    Fixing identation.
    
    Adding test for Rails::Console.parse_arguments method.
    
    Fix issue 8628 for Rails::DBConsole.
This page is out of date. Refresh to see the latest.
View
7 railties/CHANGELOG.md
@@ -4,6 +4,13 @@
*Jiri Pospisil*
+* Environment name can be a start substring of the default environemnt names
+ (production, development, test).
+ For example: tes, pro, prod, dev, devel.
+ Fix #8628
+
+ *Mykola Kyryk*
+
* Quote column names in generates fixture files. This prevents
conflicts with reserved YAML keywords such as 'yes' and 'no'
Fix #8612
View
11 railties/lib/rails/commands/console.rb
@@ -24,11 +24,20 @@ def parse_arguments(arguments)
if arguments.first && arguments.first[0] != '-'
env = arguments.first
- options[:environment] = %w(production development test).detect {|e| e =~ /^#{env}/} || env
+ if available_environments.include? env
+ options[:environment] = env
+ else
+ options[:environment] = %w(production development test).detect {|e| e =~ /^#{env}/} || env

Why not use available_environments here as well? Also, I think a single check would be enough, as detect would work in all cases.

#available_environments method return all environments (including any custom environments), where detect iterates over three default ones.
Previous code was great to run 'rails console p' and have 'production' environment loaded into console.
But it would work incorrectly if you have custom environment called 'pro' and try to run 'rails console pro', it would load production environment, not the custom 'pro' one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
end
options
end
+
+ private
+ def available_environments
+ Dir[Rails.root.join('config', 'environments', '*.rb')].map { |fname| File.basename(fname, '.*') }
+ end
end
attr_reader :options, :app, :console
View
10 railties/lib/rails/commands/dbconsole.rb
@@ -136,12 +136,20 @@ def parse_arguments(arguments)
if arguments.first && arguments.first[0] != '-'
env = arguments.first
- options[:environment] = %w(production development test).detect {|e| e =~ /^#{env}/} || env
+ if self.class.available_environments.include? env
+ options[:environment] = env
+ else
+ options[:environment] = %w(production development test).detect {|e| e =~ /^#{env}/} || env
+ end
end
options
end
+ def self.available_environments
+ Dir[Rails.root.join('config', 'environments', '*.rb')].map { |fname| File.basename(fname, '.*') }
+ end
+
def find_cmd_and_exec(commands, *args)
commands = Array(commands)
View
6 railties/test/commands/console_test.rb
@@ -111,6 +111,12 @@ def test_rails_env_is_development_when_argument_is_d
assert_match(/\sdevelopment\s/, output)
end
+ def test_rails_env_is_dev_when_argument_is_dev_and_dev_env_is_present
+ Rails::Console.stubs(:available_environments).returns(['dev'])
@senny Owner
senny added a note

I see the purpose of this stub but stubbing private methods from the instance under test gives me a bad feeling. I'm not completely sure what the stubbing/mocking policy from rails is but this does not feel right to me.

@rafaelfranca can you elaborate?

'available_environments' method is a private class method. It works with file system, so I stub it in the test.

Also, can anyone point me to a guide on rails tests?

@rafaelfranca Owner

For me is fine to stub private methods if the method is doing something expensive or depends of the environment.

We don't have any policy, we follow our :heart:. My :heart: is saying that for this case it is fine.

I'll take a look later on this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ options = Rails::Console.parse_arguments(['dev'])
+ assert_match('dev', options[:environment])
+ end
+
private
attr_reader :output
View
12 railties/test/commands/dbconsole_test.rb
@@ -45,6 +45,18 @@ def test_env
ENV['RAILS_ENV'] = "test"
end
+ def test_rails_env_is_development_when_argument_is_dev
+ Rails::DBConsole.stubs(:available_environments).returns(['development', 'test'])
+ options = Rails::DBConsole.new.send(:parse_arguments, ['dev'])
+ assert_match('development', options[:environment])
+ end
+
+ def test_rails_env_is_dev_when_argument_is_dev_and_dev_env_is_present
+ Rails::DBConsole.stubs(:available_environments).returns(['dev'])
+ options = Rails::DBConsole.new.send(:parse_arguments, ['dev'])
+ assert_match('dev', options[:environment])
+ end
+
def test_mysql
dbconsole.expects(:find_cmd_and_exec).with(%w[mysql mysql5], 'db')
start(adapter: 'mysql', database: 'db')
Something went wrong with that request. Please try again.