Refactored invalid range validation for String#delete! and String#squeeze! #1736

wants to merge 7 commits into


None yet
3 participants

antekpiechnik commented May 19, 2012

As suggested by @evanphx here 1835272

I moved the validation of the input for String#delete! and String#squeeze! methods into String#count_table. This allowed for unification of implementation of those methods across 1.8 and 1.9. The validation also now has some ruby-version-specific specs. I changed the validation messages to a more specific one, and added validation error message specs away from RubySpec (as suggested by @dbussink).

String#squeeze! validation itself was fixed (checking all provided arguments now), for which (and String#delete!) I added specific RubySpec specs in separate commits.

This also resolves/contains this pull request: #1735

antekpiechnik added some commits May 19, 2012

@antekpiechnik antekpiechnik Added RubySpec for invalid range given as second parameter in String#…
…squeeze in 1.9
@antekpiechnik antekpiechnik Added RubySpec for invalid range given as second parameter in String#…
@antekpiechnik antekpiechnik Marked the new squeeze spec as failing for ci ea0f27f
@antekpiechnik antekpiechnik Added specs to cover different behaviour of String#count_table in 1.8…
… and 1.9.

Specs that had illegal input have been separated into two ruby versions, and
respective valid-input cases have been provided for both versions.
@antekpiechnik antekpiechnik Refactored input validation for String#squeeze and String#delete in 1.9.
Moved the validation to String#count_tables from String#squeeze and String#delete
respectively and separated its implementations for both 1.8 and 1.9.
@antekpiechnik antekpiechnik Unified the implementation of String#delete! and String#squeeze! betw…
…een 1.8 and 1.9.

This pull request passes (merged a688114 into 95bb2fe).

@dbussink dbussink and 1 other commented on an outdated diff May 19, 2012

@@ -22,6 +22,38 @@ def codepoints
alias_method :each_codepoint, :codepoints
+ def count_table(*strings)
+ strings.each do |string|
+ if string =~ /.+\-.+/
+ ranges_found = string.scan(/\w{1}\-\w{1}/)
+{ |range| range.gsub(/-/, '').split('') }.each do |range_array|
+ raise ArgumentError, "invalid range \"#{range_array.join('-')}\" in string transliteration" unless range_array == range_array.sort
+ end
+ end
+ end
+ table = String.pattern 256, 1
+ i, size = 0, strings.size
+ while i < size

dbussink May 19, 2012


Why not validate here? Since here we already walk through the list of strings, now we walk it twice and that's probably more confusing.


antekpiechnik May 20, 2012


Good point, I fixed that in antekpiechnik@2de7d1a

This pull request passes (merged 2de7d1a into 95bb2fe).


antekpiechnik commented May 23, 2012

Hey @dbussink, is there anything else this pull request needs in order to get merged ?


dbussink commented May 26, 2012

I find the expression still very confusing. Isn't it a better idea to push it further down into tr_expand? There it already determines whether the expression is valid or not so the logic should be much simpler if it's integrated there.


dbussink commented May 26, 2012

If you push it own into tr_expand! it means that it is also fixed for String#tr, since that also has a very similar failing tag. It does mean you probably have to change it in the C++ primitive.

@dbussink dbussink added a commit that referenced this pull request May 26, 2012

@dbussink dbussink Cleanup String#squeeze! and String#delete!
With the change to the String#tr_expand! primitive these custom changes
are no longer necessary. This closes pull requests #1736 and #1665. Both
pull requests had useful stuff that I reused, but both also changed much
more than necessary and also in the wrong places.

Since multiple people apparently took this on and changed things in
different places, I thought it's a good idea to solve this problem
properly now and close the open pull requests.

dbussink commented May 26, 2012

The problem in #1665 was very similar to this one and the change to the primitive was better suited for solving this problem. I did take some of the spec changes from this request so the final solution could be as simple as possible. Therefore I'm closing this pull request.

dbussink closed this May 26, 2012

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