Looks great!
From: [email protected]
[mailto:[email protected]] On Behalf Of
[email protected]
Sent: Monday, April 20, 2009 4:19 PM
To: [email protected]
Subject: Re: [Ironruby-core] File.new spec fixes
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 B. 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:
[email protected]mailto:[email protected]
[mailto:[email protected]] On Behalf Of
[email protected]mailto:[email protected]
Sent: Saturday, April 18, 2009 1:35 PM
To: [email protected]mailto:[email protected]
Subject: Re: [Ironruby-core] File.new spec fixes
Hi Shri,
Thank you for quick reply. Could you please review it at
af4aceahttp://github.com/Jirapong/ironruby/commit/af4aceaa03e14c43091e64fd7fa4ce0798dfa29f
?
Note: VS.NET ProductVersion seems to be difference on my machine.
Thank you,
-Jirapong
On Apr 19, 2009, at 12:48 AM, Shri B. 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:
[email protected]mailto:[email protected]
[mailto:[email protected]] On Behalf Of
[email protected]mailto:[email protected]
Sent: Friday, April 17, 2009 11:25 PM
To: [email protected]mailto:[email protected]
Subject: Re: [Ironruby-core] File.new spec fixes
Thank you for your review. please find formatted version at
9942b24http://github.com/Jirapong/ironruby/commit/9942b249a80586403e3cd451c4b58e3179422554
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 B. 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:
[email protected]mailto:[email protected]
[mailto:[email protected]] On Behalf Of
[email protected]mailto:[email protected]
Sent: Friday, April 17, 2009 11:40 AM
To: [email protected]mailto:[email protected]
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
[email protected]mailto:[email protected]
http://rubyforge.org/mailman/listinfo/ironruby-core
Ironruby-core mailing list
[email protected]mailto:[email protected]
http://rubyforge.org/mailman/listinfo/ironruby-core