# FYI Code Review: MutableStringOps and StringScanner **URL:** \ **Category:** IronRuby\ **Created:** [October 26, 2007, 11:09am UTC](https://www.ruby-forum.com/t/fyi-code-review-mutablestringops-and-stringscanner/118382 "2007-10-26T11:09:25Z")\ **Posts on this page:** 1\ **Page:** 1
**Author:** ![John\_LSSDLR](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/j/5f8ce5/32.png) [@John\_LSSDLR](https://www.ruby-forum.com/u/John_LSSDLR)\ **Post date:** [October 26, 2007, 11:09am UTC](https://www.ruby-forum.com/t/fyi-code-review-mutablestringops-and-stringscanner/118382/1 "2007-10-26T11:09:25Z")
A big thanks to Curt H. who submitted some patches for both MutableStringOps and StringScanner! Curt’s patch was our first external contribution, and it took a while because our process isn’t quite streamlined for this process quite yet. I spent a bunch of time debugging the sync code in my Rakefile since this is the first time that we’ve pushed changes into our Merlin repository from the outside. Future patches should be much easier. Here’s some general comments about the changeset which is going through our SNAP queue tonight. 1. Our current naming convention for tests that will be run automatically via rake test is test\_zzz.rb. I’m not happy with this (I’d rather prefer zzz\_test.rb) but our existing test infrastructure uses this convention now. 2. If you’re submitting a patch, it will be useful to run “rake specs” as well. We currently don’t ‘count’ these specs as official tests since we’re failing a high % of them now. However, I suspect it won’t be long before we’ll make them official (aka your checkin will be rejected if it doesn’t pass all specs), so it’s a good idea to start running the specs against your code today. 3. Please do a quick pass (CTRL-A, CTRL-K, CTRL-F) over your code before submitting. We use 4 spaces instead of tabs for indentation, and we have open curlies on the same line. VS 2005+ will automatically reformat your code using those conventions. Some specific comments: 1. Curt uncovered a bug in our implementation of Range#each. He correctly delegated his implementation of String#upto to Range#each, but Ruby has bizarre behavior with respect to this range “9”…“A”. If you iterate over this, it will only return “9”, not an in infinite loop like you might expect. 2. If you’re implementing any methods in MutableString, I wouldn’t spend much time worrying about efficiency. We’re going to be rewriting much of the code down the road when we migrate that String type over to a byte array from its current char array implementation today. Emphasize correctness and clarity so that we can make sure that the behavior is correct and compatible with MRI for when we do the rewrite. 3. For non-builtin types like StringScanner, we still need to implement class attributes, and change the behavior of require to ‘load’ these types on the fly. For the time being, however, it’s OK to just pretend these are built-in types until we add this functionality to the language. 4. I think we’ll need to beef up the tests for StringScanner in the future to look for more corner cases. We do have code coverage test passes that we do from time to time - perhaps Haibo can fill in here about what our plans are in this area. 5. Once Tomas’ latest singleton checkin makes it into the sources (this fixes def obj.method()) definitions, we’ll take a harder look at the spec failures in the specs for the String. 6. Ruby does a lot of ‘protocol’ stuff for things like implicit conversions. It’s a good idea to use the Protocol.\* methods rather than implementing them each time. The specs do a good job of documenting observable protocols in the libraries, so if you don’t follow the protocol, the specs will generally catch and flag it as a failure. Also, make sure that you throw the correct exceptions for things like missing blocks (cf String#upto). Again, the specs will generally catch omissions as well. Other than that, looks good! Thanks again for your contribution! -John