Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix some railties test warnings #10078

Merged
merged 1 commit into from

4 participants

@vipulnsward

Fix some railties test warnings
1. Initialize variable
2. Encapsulate variable in ()

railties/test/generators/plugin_new_generator_test.rb
@@ -124,7 +124,7 @@ def test_ensure_that_skip_active_record_option_is_passed_to_app_generator
run_generator [destination_root, "--skip_active_record"]
assert_no_file "test/dummy/config/database.yml"
assert_file "test/test_helper.rb" do |contents|
- assert_no_match /ActiveRecord/, contents
+ assert_no_match (/ActiveRecord/), contents

Just add () in the assert_no_match method call:

assert_no_match(/ActiveRecord/, contents)

Updating

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
activesupport/lib/active_support/log_subscriber.rb
@@ -53,6 +53,7 @@ class LogSubscriber
class << self
def logger
+ @logger ||= nil
if defined?(Rails) && Rails.respond_to?(:logger)
@logger ||= Rails.logger
end

If you move the @logger ||= check before the if, would that remove the warning?

I din't get you. The above does remove the warning. Any different way that I should try?

Something like that?

def logger
  @logger ||= if defined?(Rails) && Rails.respond_to?(:logger)
    Rails.logger
  end
end

Oh, cool. It does. changing

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

:+1:

@carlosantoniodasilva carlosantoniodasilva merged commit 77a3bc4 into rails:master
@carlosantoniodasilva

Thanks.

@spastorino spastorino commented on the diff
activesupport/lib/active_support/log_subscriber.rb
@@ -53,10 +53,9 @@ class LogSubscriber
class << self
def logger
- if defined?(Rails) && Rails.respond_to?(:logger)
- @logger ||= Rails.logger
+ @logger ||= if defined?(Rails) && Rails.respond_to?(:logger)
+ Rails.logger
@spastorino Owner

isn't this @logger ||= defined?(Rails) && Rails.respond_to?(:logger) && Rails.logger better?

Might be, but then you need to read the entire line to see that Rails.logger is the actual value that gets set. I think the if condition makes it a little more clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 3, 2013
  1. @vipulnsward
This page is out of date. Refresh to see the latest.
View
5 activesupport/lib/active_support/log_subscriber.rb
@@ -53,10 +53,9 @@ class LogSubscriber
class << self
def logger
- if defined?(Rails) && Rails.respond_to?(:logger)
- @logger ||= Rails.logger
+ @logger ||= if defined?(Rails) && Rails.respond_to?(:logger)
+ Rails.logger
@spastorino Owner

isn't this @logger ||= defined?(Rails) && Rails.respond_to?(:logger) && Rails.logger better?

Might be, but then you need to read the entire line to see that Rails.logger is the actual value that gets set. I think the if condition makes it a little more clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
- @logger
end
attr_writer :logger
View
2  railties/test/generators/plugin_new_generator_test.rb
@@ -124,7 +124,7 @@ def test_ensure_that_skip_active_record_option_is_passed_to_app_generator
run_generator [destination_root, "--skip_active_record"]
assert_no_file "test/dummy/config/database.yml"
assert_file "test/test_helper.rb" do |contents|
- assert_no_match /ActiveRecord/, contents
+ assert_no_match(/ActiveRecord/, contents)
end
end
Something went wrong with that request. Please try again.