Ironruby core - Jun 2008 - Code Review: YamlTestsAndStream

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



--

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

I was waiting to see if this was fixed in the latest drop... but the build
output for the YAML library seems a little... wonky compared to the rest of
the projects.  Seems like it should be the same as IronRuby.Libraries. Bug?

On Fri, Jun 13, 2008 at 8:58 PM, Tomas Matousek <
Tomas.Matousek at microsoft.com> wrote:
>  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 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.
>
>
>
> --
>
> Oleg
>
> _______________________________________________
> Ironruby-core mailing list
> Ironruby-core at rubyforge.org
> http://rubyforge.org/mailman/listinfo/ironruby-core
>
>

-- 
Michael Letterle
[Polymath Prokrammer]
http://blog.prokrams.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://rubyforge.org/pipermail/ironruby-core/attachments/20080613/e828e48b/attachment.html>