1)

The lookup for Stream constant is slightly different in Ruby:



require 'YAML'



class Module

  def const_missing name

    puts "missing #{name}"

    $S

  end

end



module YAML

  $S = Stream

  remove_const "Stream"

end



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.



2)

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

end



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



Tomas



-----Original Message-----
From: Oleg Tkachenko
Sent: Friday, June 13, 2008 5:09 PM
To: IronRuby External Code Reviewers
Cc: ironruby-core@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.



--

Oleg
_______________________________________________
Ironruby-core mailing list
Ironruby-core@rubyforge.org
http://rubyforge.org/mailman/listinfo/ironruby-core

Reply via email to