Simplify String#mb_chars and fix documentation. #8261

Merged
merged 2 commits into from Nov 28, 2012

4 participants

@steveklabnik
Ruby on Rails member

Opened for feedback.

This came up due to #7798. I figured that if the if was a 1.8 to 1.9 difference, we should not just fix docs, but remove the if. Now one test fails:

  1) Failure:
test_methods_are_forwarded_to_wrapped_string_for_byte_strings(MultibyteCharsTest) [/Users/steve/src/rails/activesupport/test/multibyte_chars_test.rb:50]:
--- expected
+++ actual
@@ -1 +1 @@
-String
+ActiveSupport::Multibyte::Chars

I'm not sure this test is good, though: do we really want to proxy .class?

@josevalim
Ruby on Rails member

/cc @norman

@norman norman referenced this pull request Nov 18, 2012
Closed

mb_chars and Ruby 1.9.3+ #7798

@norman

Looks good to me. I hang my head in abject shame for not noticing this sooner. ;-)

Proxying class seems wrong to me. Does changing that break chaining method calls?

@steveklabnik
Ruby on Rails member

I have no idea.

@steveklabnik
Ruby on Rails member

Since my commit is the fix, should I change the test so that it passes and then open another that does not proxy #class?

@carlosantoniodasilva
Ruby on Rails member

I think it's fine to do it here, it can be in a different commit.

@steveklabnik
Ruby on Rails member

Okay. I've fixed the test, so it passes. I wrote a new test to check the proxy-ing of class:

diff --git a/activesupport/test/multibyte_chars_test.rb b/activesupport/test/multibyte
index 4bfd210..a8c61af 100644
--- a/activesupport/test/multibyte_chars_test.rb
+++ b/activesupport/test/multibyte_chars_test.rb
@@ -50,6 +50,10 @@ class MultibyteCharsTest < ActiveSupport::TestCase
     assert_equal BYTE_STRING.length, BYTE_STRING.mb_chars.length
   end

+  def test_class_is_not_forwarded
+    assert_equal BYTE_STRING.mb_chars.class, ActiveSupport::Multibyte::Chars
+  end
+

and now I get this unrelated failure:

  def test_consumes_utf8_strings
    assert @proxy_class.consumes?(UNICODE_STRING)
    assert @proxy_class.consumes?(ASCII_STRING)
    assert !@proxy_class.consumes?(BYTE_STRING) # => this fails
  end

How strange...any ideas on why adding a test would cause an unrelated test to fail?

steveklabnik added some commits Nov 28, 2012
@steveklabnik steveklabnik Fix documentation for String#mb_chars.
This documentation has been out of date.
27b79f4
@steveklabnik steveklabnik Simplify String#mb_chars and stop proxying #class
This behavior mattered under Ruby 1.8, but that doesn't matter now
that we don't support it.

In addition, we don't want to proxy the #class method. A test was added
to prevent against regressions.
bd85260
@steveklabnik
Ruby on Rails member

I've fixed the issue with the test, added in a CHANGELOG entry.

@carlosantoniodasilva carlosantoniodasilva merged commit e38d310 into rails:master Nov 28, 2012
@steveklabnik
Ruby on Rails member

:metal: ❤️

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