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. http://yaml.org/spec/history/2002-10-31.html#preview-family). 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. -- Oleg -------------- next part -------------- A non-text attachment was scrubbed... Name: YamlFixes.diff Type: application/octet-stream Size: 31168 bytes Desc: YamlFixes.diff URL: <http://rubyforge.org/pipermail/ironruby-core/attachments/20080723/5f51f8d7/attachment-0001.obj>
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. -----Original Message----- From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Oleg Tkachenko Sent: Wednesday, July 23, 2008 1:35 AM To: IronRuby External Code Reviewers Cc: ironruby-core at rubyforge.org Subject: [Ironruby-core] Code Review: YamlFixes 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. http://yaml.org/spec/history/2002-10-31.html#preview-family). 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. -- Oleg
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.
3) 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
-----Original Message-----
From: Oleg Tkachenko
Sent: Wednesday, July 23, 2008 1:35 AM
To: IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: Code Review: YamlFixes
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.
http://yaml.org/spec/history/2002-10-31.html#preview-family).
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.
--
Oleg
Thanks for comments, guys!
3) some old rudimentary stuff, will clean it out.
--
Oleg
-----Original Message-----
From: Tomas Matousek
Sent: Wednesday, July 23, 2008 9:31 AM
To: Oleg Tkachenko; IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: YamlFixes
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.
3) 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
-----Original Message-----
From: Oleg Tkachenko
Sent: Wednesday, July 23, 2008 1:35 AM
To: IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: Code Review: YamlFixes
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.
http://yaml.org/spec/history/2002-10-31.html#preview-family).
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.
--
Oleg
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 -----Original Message----- From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Curt Hagenlocher Sent: Wednesday, July 23, 2008 9:11 AM To: ironruby-core at rubyforge.org Subject: Re: [Ironruby-core] Code Review: YamlFixes 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. -----Original Message----- From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Oleg Tkachenko Sent: Wednesday, July 23, 2008 1:35 AM To: IronRuby External Code Reviewers Cc: ironruby-core at rubyforge.org Subject: [Ironruby-core] Code Review: YamlFixes 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. http://yaml.org/spec/history/2002-10-31.html#preview-family). 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. -- Oleg _______________________________________________ Ironruby-core mailing list Ironruby-core at rubyforge.org http://rubyforge.org/mailman/listinfo/ironruby-core
Ah, right. Looks good. AddTicks is a reasonably cheap operations so there''s probably no compelling reason to special-case (microsecond == 0). -----Original Message----- From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Oleg Tkachenko Sent: Wednesday, July 23, 2008 11:49 PM To: ironruby-core at rubyforge.org Subject: Re: [Ironruby-core] Code Review: YamlFixes 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 -----Original Message----- From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Curt Hagenlocher Sent: Wednesday, July 23, 2008 9:11 AM To: ironruby-core at rubyforge.org Subject: Re: [Ironruby-core] Code Review: YamlFixes 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. -----Original Message----- From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Oleg Tkachenko Sent: Wednesday, July 23, 2008 1:35 AM To: IronRuby External Code Reviewers Cc: ironruby-core at rubyforge.org Subject: [Ironruby-core] Code Review: YamlFixes 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. http://yaml.org/spec/history/2002-10-31.html#preview-family). 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. -- Oleg _______________________________________________ Ironruby-core mailing list Ironruby-core at rubyforge.org http://rubyforge.org/mailman/listinfo/ironruby-core _______________________________________________ Ironruby-core mailing list Ironruby-core at rubyforge.org http://rubyforge.org/mailman/listinfo/ironruby-core