Update String#squeeze for 19 #1256
Conversation
String#squeeze should throw an ArgumentError when given a bad range (e.g., 'e-b').
|
Hey @charlietanksley - looks good to me, but one more thing before merging: usually when a method from Could you add that change too? |
|
One thing you should also look at, is the behavior of squeeze! (so with the bang). If they also changed the behavior of that in 1.9, that might be a better place to add it since squeeze uses squeeze! in Rubinius. |
|
I wrote a quick intro for fixing 1.9 specs: https://gist.github.com/1289312 |
Since String#squeeze and String#squeeze! should both throw an ArgumentError when the range argument is backward, I moved the error into the String#squeeze! method and removed it from the String#squeeze method (which is defined in terms of the banged method).
|
Thanks for the feedback. @dbussink I wondered about that, but didn't see a spec so I stuck the error in squeeze. But a quick test in irb revealed that you are right, so I moved the error down into squeeze! Question: should there be a spec for this behavior in squeeze!? @abyx Done. I left squeeze in common, since I moved the error from that to squeeze!, but moved squeeze! appropriately. @burningTyger That was really helpful. Thanks. |
|
This means indeed there should be a spec for squeeze! too. Fixing things for 1.9 will undoubtly uncover things that need additional specs |
String#squeeze! should throw an error in 1.9 if the range given is 'out of sequence'.
|
Okay. I've written (well, copied!) the 1.9 spec for squeeze! and everything works there. I think that should be it (well, that is everything we've talked about here; do let me know if there is anything else I missed). I did not write the 1.8 spec yet. I'm working on that, but (a) it seems unrelated enough that it shouldn't be in this pull request, and (b) the spec is failing, and I don't want to introduce a failing spec here. I'm trying to sort out what is going on there and will produce another pull request with that spec in the not-so-distant future. |
|
After talking with brixen on #rubinius, I've cleaned this up (so it isn't so many commits!) and am closing it and submitting a new request. |
This patch does what the title says: it makes the String#squeeze spec pass on 1.9. The spec only specified what error should be thrown, so I copied the message that gets thrown from MRI 1.9.2.
This is my first pull request here, and I'm a little unsure about it, so I'm going to be a bit explicit about what I did to make sure I did things correctly. Sorry if this message seems verbose. :)
So that the method would only be implemented in 1.9 (and not mess up the 1.8 behavior), I copied the body of the current String#squeeze method into the
string19.rbfile. After adding code to that to make it throw the appropriate error, I ranbin/mspec -tx19 spec/ruby/core/string/squeeze_spec.rbto make sure it worked and thenbin/mspec spec/ruby/core/string/squeeze_spec.rbandrake specto make sure it didn't break anything. I couldn't tell if there were any other tests I should run to make sure I didn't break anything else, so that I where I stopped.I'm more than happy to make any changes you'd like to clean this up. I couldn't figure out an elegant way to test whether the sequence was out of order, but I did some up with something that works!