Ironruby core - Oct 2007 - FYI Code Review: MutableStringOps and StringScanner

A big thanks to Curt Hagenlocher 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