FYI Code Review: MutableStringOps and StringScanner

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

This forum is not affiliated to the Ruby language, Ruby on Rails framework, nor any Ruby applications discussed here.

| Privacy Policy | Terms of Service | Remote Ruby Jobs