[Ironruby-core] Code Review: YamlTestsAndStream

Tomas Matousek Tomas.Matousek at microsoft.com
Fri Jun 13 20:58:45 EDT 2008


The lookup for Stream constant is slightly different in Ruby:

require 'YAML'

class Module

  def const_missing name

    puts "missing #{name}"




module YAML

  $S = Stream

  remove_const "Stream"


p YAML::load_stream(File.new("C:\\temp\\yaml.txt"))


To achieve the same behavior use RubyUtils.GetConstant(context, self, "Stream", false) instead of TryLookupConstant. This method also throws the right exception on failure.


In YAML.Stream.Emit:

return RubyYaml.DumpAll(context, RubyUtils.GetExecutionContext(context).GetModule(typeof(YamlStreamOps)), self.Documents, io);

Since DumpAll method doesn't use "self" parameter, it's unnecessary to create one. It would be better to define a new method that doesn't take the argument and call it from Emit.

3) YamlStream fields seem to be readonly, yet they are not marked as such. Also, it seems that you expect _documents to be non-nullable (mark it by /*!*/ if so).

4) YAML::Stream could be used as a base class in Ruby:

class MyStream < YAML::Stream


To enable that, YamlStream class needs to have a default constructor (could be protected).


-----Original Message-----
From: Oleg Tkachenko
Sent: Friday, June 13, 2008 5:09 PM
To: IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: Code Review: YamlTestsAndStream

tfpt review "/shelveset:YamlTestsAndStream;REDMOND\olegtkac"

Comment  :

  Adds some basic unit tests (taken from yaml spec and ruby 1.8.6).

  Fixes a bug with registration of predefined ruby contructors, which allows to load more that one yaml document.

  Checks if YAML::Stream class is avaliable and fails gracefully if not.

  Introduces YAML::Stream class implementation.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://rubyforge.org/pipermail/ironruby-core/attachments/20080613/5dd4944a/attachment.html>

More information about the Ironruby-core mailing list