Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate Unicode#downcase/upcase/swapcase. #34123

Merged
merged 2 commits into from Oct 12, 2018

Conversation

frodsan
Copy link
Contributor

@frodsan frodsan commented Oct 8, 2018

  • Deprecates ActiveSupport::Multibyte::Unicode#downcase/upcase/swapcase in favor of using String methods directly.
  • Removes ActiveSupport::Multibyte::Chars wrappers around the following String methods: #downcase, #upcase, #swapcase, and #capitalize.

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@frodsan frodsan force-pushed the deprecate_unicode_string_wrappers branch from aa7f113 to b5b7aa0 Compare October 8, 2018 09:06
@jeremy
Copy link
Member

jeremy commented Oct 8, 2018

Related to #26743 and #26403.

When you already have a Chars object, this deprecation message tells you to use a method from a different class. How will a user understand this and what should they do?

Is the underlying intent to deprecate ActiveSupport::Multibyte::Chars itself?

@frodsan
Copy link
Contributor Author

frodsan commented Oct 9, 2018

The intent is to deprecate the wrappers for the String methods (upcase, downcase, swapcase) in AS::Multibyte::Unicode. These methods are used in AS::Multibyte::Chars so I changed those to just use the String methods directly. The warning only appears if you use the Unicode methods directly. I added a test case to show this behavior, but here is the output in irb:

>> ActiveSupport::Multibyte::Unicode.downcase("DEPRECATED")
DEPRECATION WARNING: ActiveSupport::Multibyte::Unicode#downcase is deprecated and will be removed from Rails 6.1. Use String#downcase instead. (called from block (3 levels) in run at /Users/frodsan/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/minitest-5.11.3/lib/minitest/test.rb:98)
deprecated
>> "NOTDEPRECATED".mb_chars.downcase.to_s
notdeprecated

I think these could be the wrapper methods referred here: #26743 (comment)

@frodsan frodsan force-pushed the deprecate_unicode_string_wrappers branch from 792876c to aa629d1 Compare October 9, 2018 08:10
@jeremy
Copy link
Member

jeremy commented Oct 9, 2018

That sounds good. Let's deprecate these methods together as a group and change the warning message to suggest using String directly since the ActiveSupport::Multibyte::Unicode wrapper is no longer needed.

@jeremy
Copy link
Member

jeremy commented Oct 9, 2018

/cc @mtsmfm @amatsuda

@amatsuda
Copy link
Member

+1 from me!

@mtsmfm
Copy link
Contributor

mtsmfm commented Oct 10, 2018

👍

@frodsan frodsan force-pushed the deprecate_unicode_string_wrappers branch 2 times, most recently from 1c4fc82 to 3e7b2ac Compare October 11, 2018 06:31
@frodsan
Copy link
Contributor Author

frodsan commented Oct 11, 2018

@jeremy I made the changes. I hope I got it right :) ... I also removed the method definitions from Multibyte::Chars because that logic is already being handled by #method_missing.

@frodsan
Copy link
Contributor Author

frodsan commented Oct 11, 2018

I'm also removing the wrapper around String#capitalize after discussing this with @mtsmfm (#26743 (comment))

@mtsmfm
Copy link
Contributor

mtsmfm commented Oct 11, 2018

reverse can be chars(@wrapped_string.scan(/\X/).reverse.join)

Copy link
Member

@jeremy jeremy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @frodsan. For those interested, let's look at deprecating Unicode.normalize and changing Chars#reverse implementation in new PRs.

@jeremy
Copy link
Member

jeremy commented Oct 11, 2018

Looks like a failing test remains:

MultibyteCharsUTF8BehaviourTest#test_method_works_for_proxyed_methods:
FrozenError: can't modify frozen String
    /home/travis/build/rails/rails/activesupport/lib/active_support/multibyte/chars.rb:61:in `capitalize!'
    /home/travis/build/rails/rails/activesupport/lib/active_support/multibyte/chars.rb:61:in `method_missing'
    /home/travis/build/rails/rails/activesupport/test/multibyte_chars_test.rb:481:in `call'
    /home/travis/build/rails/rails/activesupport/test/multibyte_chars_test.rb:481:in `test_method_works_for_proxyed_methods'

@frodsan frodsan force-pushed the deprecate_unicode_string_wrappers branch from 6cc79ce to 16e3b65 Compare October 12, 2018 05:47
@frodsan
Copy link
Contributor Author

frodsan commented Oct 12, 2018

@jeremy I fixed the failing test.

I just found that the bang version of these methods will raise FrozenError when the wrapped string is frozen. I think that should be expected:

>> (-"hello").mb_chars.chomp!
FrozenError (can't modify frozen String)
>> (-"hello").mb_chars.gsub!("m", "")
FrozenError (can't modify frozen String)
>> (-"hello").mb_chars.downcase!
FrozenError (can't modify frozen String)

I'll work on the Unicode#normalize deprecation.

@jeremy jeremy merged commit 619b2ae into rails:master Oct 12, 2018
frodsan added a commit to frodsan/rails that referenced this pull request Oct 16, 2018
Use \X meta character directly to get grapheme clusters.

Thanks to @mtsmfm for the tip:
rails#34123 (comment)

r? @jeremy
jeremy pushed a commit that referenced this pull request Oct 16, 2018
Use \X meta character directly to get grapheme clusters.

Thanks to @mtsmfm for the tip:
#34123 (comment)

r? @jeremy
suketa added a commit to suketa/rails_sandbox that referenced this pull request Jun 29, 2019
Deprecate Unicode#downcase/upcase/swapcase
rails/rails#34123
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants