Code Review: YamlFixes

Ruby IronRuby
1

tfpt review “/shelveset:YamlFixes;REDMOND\olegtk”
Comment :
Removes obsolete YamlTest project from the solution.
Fixes DateTime serialization and loading of timestamps.
Fixes nil serialization.
Implements YAML::add_domain_type() - user controlled parsing of domain
types.
Fixes loading binary data in Ruby context.
Implements loading dates - instantiates Date class (from date.rb
curently) instead of DateTime.
Adds “require ‘date’” into yaml.rb to be compatible with MRI (it makes
loading yaml much slower currently - need to implement Date class).
Fixes bug with serializing a hash with a nil as a key.
Implements loading abbreviated types (looks like obsoleted yaml 1.0
only feature, but MRI tests it.
YAML Ain't Markup Language (YAML) (tm) 1.0).
Splits SafeConstructor.ConstructYamlTimestamp into two methods so
RubyConstructor can override Date construction.
Fixes Time.at() - the argument must be a double type.
Fixes Time.local() and Time.utc() - usec argument is actually
microseconds, not milliseconds.

2

Microseconds are being truncated to milliseconds instead being rounded.
The least painful way to fix this is to .AddMilliseconds(1) to the
result if usec >= 500. (I might be okay with the truncation if it’s
acknowledged in a comment.)

There’s a typo in a comment added to BaseConstructor.cs.

I’m not qualified to comment on the more YAML-specific stuff.

3

  1. add_domain_type should probably be split into 2 overloads - one for
    Regex and other for object converted to a string since the
    implementation is different.

  2. Do not use dynamic sites for block invocation (RubyConstructor.cs):

return _Block.Invoke(ctor.GetContext(), _block,
MutableString.Create(tag), ctor.ConstructPrimitive(node));

Use _Block.Yield.

  1. Representer.cs: why do we compare to two different “Null” objects?

             if (key == BaseConstructor.NullObjectKey) {
             key = null;
         } else {
             key = BaseSymbolDictionary.ObjToNull(key);
         }

Other than that looks good.

Tomas

4

Well, actually we don’t need to truncate or round altogether as DateTime
has 0.1 microsecond precision. Here is a better way:

return new DateTime(year, month, day, hour, minute, second,
DateTimeKind.Utc).AddTicks(microsecond * 10);

If that looks ok, I’m going to submit the shelveset.

–
Oleg

5

Thanks for comments, guys!
3) some old rudimentary stuff, will clean it out.

–
Oleg

6

Ah, right. Looks good. AddTicks is a reasonably cheap operations so
there’s probably no compelling reason to special-case (microsecond ==
0).