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

Add support for multiple encodings in String.blank? #31049

Merged
merged 1 commit into from Jan 4, 2018

Conversation

@gwincr11
Copy link
Contributor

@gwincr11 gwincr11 commented Nov 4, 2017

Motivation:

  • When strings are encoded with .encode("UTF-16LE") .blank? throws
    an Encoding::CompatibilityError exception.
  • We tested multiple implementation to see what the fastest
    implementation was, rescueing the execption seems to be the fastest
    option we could find.

Related Issues:

Changes:

  • Add a rescue to catch the exception.
  • Added a Concurrent::Map to store a cache of encoded regex objects
    for requested encoding types.
  • Use the new Concurrent::Map cache to return the correct regex for
    the string being checked.

cc @matthewd for review.

@rails-bot
Copy link

@rails-bot rails-bot commented Nov 4, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kaspth (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.

activesupport/lib/active_support/core_ext/object/blank.rb Outdated
@@ -1,5 +1,8 @@
# frozen_string_literal: true


require_relative "../regexp"

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 6, 2017
Member

Is this fine needed? There is no regexp file in the object folder.

This comment has been minimized.

@gwincr11

gwincr11 Nov 7, 2017
Author Contributor

This line seems to have come from this commit: gwincr11@8da30ad

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 9, 2017
Member

The line 6 is doing the same thing. You can remove it

activesupport/lib/active_support/core_ext/object/blank.rb Outdated
@@ -1,5 +1,8 @@
# frozen_string_literal: true


require_relative "../regexp"

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 9, 2017
Member

The line 6 is doing the same thing. You can remove it

@gwincr11
Copy link
Contributor Author

@gwincr11 gwincr11 commented Nov 13, 2017

@rafaelfranca ok it has been removed.

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Looks good to me. After this last change I requested could you squash your commits?

activesupport/lib/active_support/core_ext/object/blank.rb Outdated
@@ -1,5 +1,5 @@
# frozen_string_literal: true

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 13, 2017
Member

We need an empty line after the comment.

Motivation:
  - When strings are encoded with `.encode("UTF-16LE")` `.blank?` throws
  an `Encoding::CompatibilityError` exception.
  - We tested multiple implementation to see what the fastest
  implementation was, rescueing the execption seems to be the fastest
  option we could find.

Related Issues:
  - #28953

Changes:
  - Add a rescue to catch the exception.
  - Added a `Concurrent::Map` to store a cache of encoded regex objects
  for requested encoding types.
  - Use the new `Concurrent::Map` cache to return the correct regex for
  the string being checked.
@gwincr11 gwincr11 force-pushed the gwincr11:cg-blank branch Nov 18, 2017
@gwincr11
Copy link
Contributor Author

@gwincr11 gwincr11 commented Nov 18, 2017

@rafaelfranca I followed the squash instruction from the Rails Contributing guide, I am not sure the end result is what is desired??

@matthewd
Copy link
Member

@matthewd matthewd commented Nov 18, 2017

Indeed not. Looks like you've successfully squashed your change down to one commit, but ended up with other new commits rebased on top of it.

You can discard those extra commits with (while on your cg-blank branch, with no uncommitted changes):
git reset --hard 7b44ffa21efe3b9608254701ffdf44f743b1a324

That'll forget all the commits since 7b44ffa, which is your change.

@gwincr11 gwincr11 force-pushed the gwincr11:cg-blank branch to 7b44ffa Nov 18, 2017
@kamipo kamipo merged commit c7b8327 into rails:master Jan 4, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.