http://github.com/Jirapong/ironruby/commit/e5dfbef094e4712d8859f5cda142b608dafe3b16
Makes Code refactor as reviewed. see inline answer.
Files changed:
• Merlin/External.LCA_RESTRICTED/Languages/IronRuby/mspec/rubyspec/
core/kernel/open_spec.rb
• Merlin/Main/Languages/Ruby/Libraries.LCA_RESTRICTED/Builtins/Errno.cs
• Merlin/Main/Languages/Ruby/Libraries.LCA_RESTRICTED/Builtins/
KernelOps.cs
• Merlin/Main/Languages/Ruby/Libraries.LCA_RESTRICTED/
Initializers.Generated.cs
• Merlin/Main/Languages/Ruby/Ruby/Builtins/Exceptions.cs
• Merlin/Main/Languages/Ruby/Ruby/Builtins/RubyErrno.cs
Thanks,
-Jirapong
On Apr 19, 2009, at 4:43 AM, Shri Borde wrote:
Looks good! The only comment is that in IronRuby.dll (an in the
libraries too wherever possible), the code should be “good” C# code,
and should have as few Ruby-isms in it as possible. In RubyErrno.cs,
you added a static class IronRuby.Builtins.RubyErrno with inner
exception classes like ExistError. You would normally not declare
exceptions as inner classes, and so you should not do that here
either. Instead, just keep ExistError etc in the IronRuby.Builtins
namespace (just like SystemExit, etc in Ruby\Builtins\Exceptions.cs)
or use a namespace like “IronRuby.Builtins.RubyErrno” if you want.
But using a static class serves no useful purpose and so should be
avoided.
I have move Errno's exceptions to exceptions.cs and makes RubyErrno.cs
to be only a helper class like IronRuby.Runtime.RubyExceptions class. -
JN
Ofcourse, in Libraries.LCA_RESTRICTED\Builtins\Errno.cs, you will
need to use inner classes to match the Ruby class hierarchy, but
that is a different matter…
I see that you removed “if (path.Empty)” from FileOps.cs and moved
it to File.cs, which is great! Couldn’t you do the same in KernelOps
too? You changed how KernelOps throws the exception, but the whole
“if (path.IsEmpty)” could be removed…
Done - JN
From: ironruby-core-boun...@rubyforge.org [mailto:ironruby-core-boun...@rubyforge.org
] On Behalf Of jirapong.na...@gmail.com
Sent: Saturday, April 18, 2009 1:35 PM
To: ironruby-core@rubyforge.org
Subject: Re: [Ironruby-core] File.new spec fixes
Hi Shri,
Thank you for quick reply. Could you please review it
at af4acea ?
Note: VS.NET ProductVersion seems to be difference on my machine.
Thank you,
-Jirapong
On Apr 19, 2009, at 12:48 AM, Shri Borde wrote:
You will need to add the exception classes in IronRuby.dll, and
then define XXXOps classes in IronRuby.Libraries.dll with the
[RubyClass] attribute. See FileNotFoundExceptionOps in
Libraries.LCA_RESTRICTED\Builtins\Errno.cs for an example of how
this is done.
From: ironruby-core-boun...@rubyforge.org [mailto:ironruby-core-boun...@rubyforge.org
] On Behalf Of jirapong.na...@gmail.com
Sent: Friday, April 17, 2009 11:25 PM
To: ironruby-core@rubyforge.org
Subject: Re: [Ironruby-core] File.new spec fixes
Thank you for your review. please find formatted version at 9942b24
I just do a pull request for 929e07 and 9942b24.
please see my answer inline.
Thank you,
-Jirapong
On Apr 18, 2009, at 5:51 AM, Shri Borde wrote:
Some blocks are not indented as in the second line in this example.
Could you indent those please?
+ lambda {
+ @fh = File.new(@file, File::CREAT|File::EXCL)
+ }.should raise_error(Errno::EEXIST)
0
You added try-catch to one of the overloads of
RubyFileOps.CreateFile. Could you add it to all?
Yes, It must be, but this will not longer need if we move Errno to
Ruby project.
Btw, I discussed with Tomas about the fact that many of the IO
exceptions are defined in IronRuby.Libraries.dll, but RubyFile needs
to throw those exception from IronRuby.dll. We can move the
exceptions that are needed in IronRuby.dll into IronRuby.dll. That
will avoid having to do a try-catch to translate the exception
type. Do you want to add Languages\Ruby\Ruby\Builtins\Errno.cs and
move some of the exception types there?
I'm trying to add Errno.cs with EEXIST exception in IronRuby.dll,
but I can't get its initialize code in Initializers.Generated.cs
file after run geninit. Is it possible to define
[RubyClass("EEXIST")] in IronRuby.dll?
Thanks,
Shri
From: ironruby-core-boun...@rubyforge.org [mailto:ironruby-core-boun...@rubyforge.org
] On Behalf Of jirapong.na...@gmail.com
Sent: Friday, April 17, 2009 11:40 AM
To: ironruby-core@rubyforge.org
Subject: [Ironruby-core] File.new spec fixes
http://github.com/Jirapong/ironruby/commit/929e07e27183e70a6e4bed8197430ff533610762
Fixes for core\file\new_spec.rb:
File.new raises an Errorno::EEXIST if the file exists when create a
new file with File::CREAT|File::EXCL
File.new raises an Errno::EINVAL error with File::APPEND
File.new raises an Errno::EINVAL error with File::RDONLY|File::APPEND
Files changed:
• Merlin/External.LCA_RESTRICTED/Languages/IronRuby/
mspec/ironruby-tags/core/file/new_tags.txt
• Merlin/External.LCA_RESTRICTED/Languages/IronRuby/
mspec/rubyspec/core/file/new_spec.rb
• Merlin/Main/Languages/Ruby/Libraries.LCA_RESTRICTED/
Builtins/FileOps.cs
• Merlin/Main/Languages/Ruby/Ruby/Builtins/File.cs
Thank you,
-Jirapong
_______________________________________________
Ironruby-core mailing list
Ironruby-core@rubyforge.org
http://rubyforge.org/mailman/listinfo/ironruby-core
_______________________________________________
Ironruby-core mailing list
Ironruby-core@rubyforge.org
http://rubyforge.org/mailman/listinfo/ironruby-core
_______________________________________________
Ironruby-core mailing list
Ironruby-core@rubyforge.org
http://rubyforge.org/mailman/listinfo/ironruby-core
_______________________________________________
Ironruby-core mailing list
Ironruby-core@rubyforge.org
http://rubyforge.org/mailman/listinfo/ironruby-core
