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.
-
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. -
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. -
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:
-
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. -
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. -
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. -
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. -
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. -
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