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

fix repeat of test; remove unused variable by use of each_key #9763

Merged

Conversation

vipulnsward
Copy link
Member

  1. The loop was unnecessarily repeating itself
  2. Removed unused variable by use of each_key in place of each

string.each_char do |char|
assert_match %r{^[a-zA-Z']*$}, ActiveSupport::Inflector.transliterate(string)
end
assert_match %r{^[a-zA-Z']*$}, ActiveSupport::Inflector.transliterate(string)

Choose a reason for hiding this comment

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

I wonder if there's a reason for that, or it's just historical due to ruby 1.8? Original commit: f0e754e

@fxn @norman can you please take a look?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is to get better error messages. The string created by line 20 is:

ÀÁÂÃÄÅÆÇÈÉÊËÌÍÎÏÐÑÒÓÔÕÖØÙÚÛÜÝÞßàáâãäåæçèéêëìíîïðñòóôõöøùúûüýþÿĀāĂ㥹ĆćĈĉĊċČčĎďĐđĒēĔĕĖėĘęĚěĜĝĞğĠġĢģĤĥĦħĨĩĪīĬĭĮįİıIJijĴĵĶķĸĹĺĻļĽľĿŀŁłŃńŅņŇňʼnŊŋŌōŎŏŐőŒœŔŕŖŗŘřŚśŜŝŞşŠšŢţŤťŦŧŨũŪūŬŭŮůŰűŲųŴŵŶŷŸŹźŻżŽž

In the event of a failure it's nicer to know exactly where it failed rather than "somewhere in there." A few of the characters don't actually decompose down to a letter and a diacritic, so the places where failures are most likely to arise due to refactoring the transliteration code are not intuitive to most people. I would leave this one alone.

Copy link
Member Author

Choose a reason for hiding this comment

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

@norman So instead the test should be taking place on char variable instead of string right?
Because even if we are going over the whole string, we are just keeping on doing the same thing for all the characters, if I am not wrong.
ie. we do assert_match %r{^[a-zA-Z']*$}, ActiveSupport::Inflector.transliterate(string) instead of assert_match %r{^[a-zA-Z']*$}, ActiveSupport::Inflector.transliterate(char) repeatedly. Or is this intentional too?

Copy link
Contributor

Choose a reason for hiding this comment

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

So instead the test should be taking place on char variable instead of string right?

Ah, yes that's right. I looked right past that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updating the PR to reflect this then

@vipulnsward
Copy link
Member Author

@carlosantoniodasilva @norman Fixed/Squashed it.

carlosantoniodasilva added a commit that referenced this pull request Mar 18, 2013
Fix repeat of test; remove unused variable by use of each_key
@carlosantoniodasilva carlosantoniodasilva merged commit 4e7292c into rails:master Mar 18, 2013
@carlosantoniodasilva
Copy link
Member

Thanks @Normal @vipulnsward

@vipulnsward vipulnsward deleted the fix_transliterate_test branch March 18, 2013 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants