Skip to content
This repository
Browse code

Revert "Merge pull request #7084 from LTe/logger_default_separator"

This reverts commit c08f30f, reversing
changes made to e243a8a.
  • Loading branch information...
commit e482100d6ed022d00ba31c6c4377f6f947173337 1 parent fe5b943
Aaron Patterson authored July 18, 2012
2  activesupport/lib/active_support/tagged_logging.rb
@@ -31,7 +31,7 @@ def current_tags
31 31
       def tags_text
32 32
         tags = current_tags
33 33
         if tags.any?
34  
-          tags.collect { |tag| "[#{tag}] " }.join('')
  34
+          tags.collect { |tag| "[#{tag}] " }.join
35 35
         end
36 36
       end
37 37
     end
15  activesupport/test/tagged_logging_test.rb
@@ -10,13 +10,8 @@ def flush(*)
10 10
   end
11 11
 
12 12
   setup do
13  
-    @output     = StringIO.new
14  
-    @logger     = ActiveSupport::TaggedLogging.new(MyLogger.new(@output))
15  
-    @separator  = $,
16  
-  end
17  
-
18  
-  after do
19  
-    $, = @separator
  13
+    @output = StringIO.new
  14
+    @logger = ActiveSupport::TaggedLogging.new(MyLogger.new(@output))
20 15
   end
21 16
 
22 17
   test "tagged once" do
@@ -74,10 +69,4 @@ def flush(*)
74 69
 
75 70
     assert_equal "[BCX] [Jason] Funky time\n[BCX] Junky time!\n", @output.string
76 71
   end
77  
-
78  
-  test "using the correct separator" do
79  
-    $, = "_"
80  
-    @logger.tagged("BCX", "BDX") { @logger.info "Funky time" }
81  
-    assert_equal "[BCX] [BDX] Funky time\n", @output.string
82  
-  end
83 72
 end

5 notes on commit e482100

Rafael Mendonça França
Owner

Reason?

Steve Klabnik
Collaborator

I wasn't involved in this discussion, but $, should absolutely be respected. We're not making a custom Ruby for Rails...

Aaron Patterson
Owner

If you modify a global variable, it modifies the behavior of the program, globally.

People who never set the global variable would not notice the "fix" in the previous commit. People who depended on the global variable would have broken code. Best case: nobody would notice the commit, worst case: we broke someone's code.

Aaron Patterson
Owner

TBH, a better argument to not use the $, (in this case), would be that the current code ends up with a blank space at the end of the string. It would be better if the code was changed to tags.collect { |tag| "[#{tag}]" }.join ' '.

Rafael Mendonça França
Owner

seems good. Thank you to explain.

Please sign in to comment.
Something went wrong with that request. Please try again.