Ironruby core - May 2009 - Code Review: Fix for File.print misbehavior

Commit Description:
Using the File.print method after the global input record separator had
been changed generated a different output than the MRI.

For example, consider the next code block:
$\ = ">>"
file =  File.open("a.txt","w")
file.print "One ", 1, ", Two ", 2
file.close

Output file on MRI: One 1, Two 2>>
Output file on IronRuby: One >>1>>, Two >>2>>

This commit fixes this behavior.

Commit site:
http://github.com/shayfriedman/ironruby/commit/0ccacf09b53c231f21851391d616b0fa53026197

Files changed:
Merlin/Main/Languages/Ruby/Libraries.LCA_RESTRICTED/Builtins/IoOps.cs

Thanks,
Shay.
-- 
Posted via http://www.ruby-forum.com/.

Thanks for the first patch, Shay! Couple of questions and comments.

Have you signed the contributor agreement mentioned at the top of
http://wiki.github.com/ironruby/ironruby/contributing?

Please see http://wiki.github.com/ironruby/ironruby/modifying-the-sources for
coding conventions. Opening braces should go on the same line as the
"if" statement.

You should make sure that there is a RubySpec spec for your fixes. In this case,
there should be a spec in
Merlin\External.LCA_RESTRICTED\Languages\IronRuby\mspec\rubyspec\core\io\print_spec.rb.
It does look like the example "writes each obj.to_s to the stream and
appends $\\ (if any) given multiple objects" corresponds to your fix. If
there is an existing spec, you should delete the corresponding tag from
Merlin\External.LCA_RESTRICTED\Languages\IronRuby\mspec\ironruby-tags\core\io\print_tags.txt.
Please delete the print_tags.txt file since there are not more tags left in that
file.

You did run irtests.bat, right? There are currently 5 known failures from
run.bat and 19 from the RubySpec specs (all of which should go away with the
next push from Microsoft TFS to GIT).

Thanks again!

Shri


-----Original Message-----
From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at
rubyforge.org] On Behalf Of Shay Friedman
Sent: Friday, May 15, 2009 8:20 AM
To: ironruby-core at rubyforge.org
Subject: [Ironruby-core] Code Review: Fix for File.print misbehavior

Commit Description:
Using the File.print method after the global input record separator had
been changed generated a different output than the MRI.

For example, consider the next code block:
$\ = ">>"
file =  File.open("a.txt","w")
file.print "One ", 1, ", Two ", 2
file.close

Output file on MRI: One 1, Two 2>>
Output file on IronRuby: One >>1>>, Two >>2>>

This commit fixes this behavior.

Commit site:
http://github.com/shayfriedman/ironruby/commit/0ccacf09b53c231f21851391d616b0fa53026197

Files changed:
Merlin/Main/Languages/Ruby/Libraries.LCA_RESTRICTED/Builtins/IoOps.cs

Thanks,
Shay.
-- 
Posted via http://www.ruby-forum.com/.
_______________________________________________
Ironruby-core mailing list
Ironruby-core at rubyforge.org
http://rubyforge.org/mailman/listinfo/ironruby-core

Shay has signed the agreement

JD

....there is no try
Sent from my phone. Please excuse typos and txtspk.

-----Original Message-----
From: Shri Borde <Shri.Borde at microsoft.com>
Sent: May 15, 2009 11:49 AM
To: ironruby-core at rubyforge.org <ironruby-core at rubyforge.org>
Subject: Re: [Ironruby-core] Code Review: Fix for File.print misbehavior


Thanks for the first patch, Shay! Couple of questions and comments.

Have you signed the contributor agreement mentioned at the top of
http://wiki.github.com/ironruby/ironruby/contributing?

Please see http://wiki.github.com/ironruby/ironruby/modifying-the-sources for
coding conventions. Opening braces should go on the same line as the
"if" statement.

You should make sure that there is a RubySpec spec for your fixes. In this case,
there should be a spec in
Merlin\External.LCA_RESTRICTED\Languages\IronRuby\mspec\rubyspec\core\io\print_spec.rb.
It does look like the example "writes each obj.to_s to the stream and
appends $\\ (if any) given multiple objects" corresponds to your fix. If
there is an existing spec, you should delete the corresponding tag from
Merlin\External.LCA_RESTRICTED\Languages\IronRuby\mspec\ironruby-tags\core\io\print_tags.txt.
Please delete the print_tags.txt file since there are not more tags left in that
file.

You did run irtests.bat, right? There are currently 5 known failures from
run.bat and 19 from the RubySpec specs (all of which should go away with the
next push from Microsoft TFS to GIT).

Thanks again!

Shri


-----Original Message-----
From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at
rubyforge.org] On Behalf Of Shay Friedman
Sent: Friday, May 15, 2009 8:20 AM
To: ironruby-core at rubyforge.org
Subject: [Ironruby-core] Code Review: Fix for File.print misbehavior

Commit Description:
Using the File.print method after the global input record separator had
been changed generated a different output than the MRI.

For example, consider the next code block:
$\ = ">>"
file =  File.open("a.txt","w")
file.print "One ", 1, ", Two ", 2
file.close

Output file on MRI: One 1, Two 2>>
Output file on IronRuby: One >>1>>, Two >>2>>

This commit fixes this behavior.

Commit site:
http://github.com/shayfriedman/ironruby/commit/0ccacf09b53c231f21851391d616b0fa53026197

Files changed:
Merlin/Main/Languages/Ruby/Libraries.LCA_RESTRICTED/Builtins/IoOps.cs

Thanks,
Shay.
--
Posted via http://www.ruby-forum.com/.
_______________________________________________
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

Hi Shri,

Regarding the contributor agreement - yes, I''ve signed that (Jim
Deville
helped me with that).

Regarding the coding conventions - I didn''t notice the "modifying
the
sources" page, sorrry for that... I would pay attention to that next 
time.

About RubySpec - "writes each obj.to_s to the stream and
appends $\\ (if any) given multiple objects" really seems like the 
corresponding test to my fix. What are the tags for?
The test mentioned on print_tags.txt should also pass after this commit.

About irtests - I ran it. I saw the 5 failures from run.bat but I 
couldn''t find the place where the RubySpec specs failures were 
presented. What am I doing wrong?

Thanks!
Shay.
-- 
Posted via http://www.ruby-forum.com/.

"writes each obj.to_s to the stream and appends $\\ (if any) given multiple
objects" does not have a tag for it. If it did, it would be in
Merlin\External.LCA_RESTRICTED\Languages\IronRuby\mspec\ironruby-tags\core\io\print_tags.txt,
but that file only has a tag for "writes $_.to_s followed by $\ (if any) to
the stream if no arguments given". I incorrectly thought that these were
the same test name, but clearly they are not.

It''s a mystery how "writes each obj.to_s to the stream and appends
$\\ (if any) given multiple objects" works, but your example below fails.
Could you could step into IoOps.Print for this test without your fix and see how
it works? Else, I can try it next week.

Shri


-----Original Message-----
From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at
rubyforge.org] On Behalf Of Shay Friedman
Sent: Friday, May 15, 2009 10:51 PM
To: ironruby-core at rubyforge.org
Subject: Re: [Ironruby-core] Code Review: Fix for File.print misbehavior

Hi Shri,

Regarding the contributor agreement - yes, I''ve signed that (Jim
Deville
helped me with that).

Regarding the coding conventions - I didn''t notice the "modifying
the
sources" page, sorrry for that... I would pay attention to that next 
time.

About RubySpec - "writes each obj.to_s to the stream and
appends $\\ (if any) given multiple objects" really seems like the 
corresponding test to my fix. What are the tags for?
The test mentioned on print_tags.txt should also pass after this commit.

About irtests - I ran it. I saw the 5 failures from run.bat but I 
couldn''t find the place where the RubySpec specs failures were 
presented. What am I doing wrong?

Thanks!
Shay.
-- 
Posted via http://www.ruby-forum.com/.
_______________________________________________
Ironruby-core mailing list
Ironruby-core at rubyforge.org
http://rubyforge.org/mailman/listinfo/ironruby-core

I couldn''t follow the code exactly but what I did find, was that this 
test never reached the code inside IOOps.Print.
The behavior is really strange, I tried to print the value on the screen 
and the value were really incorrect (the $\ delimiter is added after 
each argument) but the $stdout.print call inside the lambda expression 
got it correctly...

I did that:

  it "writes each obj.to_s to the stream and appends $\\ (if any) given 
multiple

objects" do
    o, o2 = Object.new, Object.new
    def o.to_s(); ''o''; end
    def o2.to_s(); ''o2''; end

    puts $stdout.print(o, o2)

    lambda { $stdout.print(o, o2) }.should 
output("#{o.to_s}#{o2.to_s}#{$\}")
  end


And it wrote "o->o2->nil" on the screen... But when I changed
the output
it to "123" the test failed and told me:
Expected:
  $stdout: "123"
      got:
  $stdout: "oo2->"

So I''m really confused... like I said, my breakpoint inside the print 
method never gets hit so I can''t really tell which "print"
method is
invoked...

Shay.
-- 
Posted via http://www.ruby-forum.com/.

The problem is because of mspec itself. In
External.LCA_RESTRICTED\Languages\IronRuby\mspec\mspec\lib\mspec\matchers\output.rb,
the code sets $stdout to a helper class (IOStub) so that it can capture the
output. However, what this means is that calls to $stdout.print that the test
makes now go to IOStub#print rather than to the real implementation of IO#print,
and this masks the bug.

A better solution would be to use StringIO in matchers\output.rb rather than
IOStub. If you are included to pursue this (which would be great), you could try
the change and/or send email to http://groups.google.com/group/rubyspec to
discuss the change. However, since this is a preexisting issue, you could leave
it for another day if you want.

So consider your change code-reviewed, and you can chose separately to pursue
(or not) the issue with matchers\output.rb.

-----Original Message-----
From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at
rubyforge.org] On Behalf Of Shay Friedman
Sent: Saturday, May 16, 2009 12:46 AM
To: ironruby-core at rubyforge.org
Subject: Re: [Ironruby-core] Code Review: Fix for File.print misbehavior

I couldn''t follow the code exactly but what I did find, was that this 
test never reached the code inside IOOps.Print.
The behavior is really strange, I tried to print the value on the screen 
and the value were really incorrect (the $\ delimiter is added after 
each argument) but the $stdout.print call inside the lambda expression 
got it correctly...

I did that:

  it "writes each obj.to_s to the stream and appends $\\ (if any) given 
multiple

objects" do
    o, o2 = Object.new, Object.new
    def o.to_s(); ''o''; end
    def o2.to_s(); ''o2''; end

    puts $stdout.print(o, o2)

    lambda { $stdout.print(o, o2) }.should 
output("#{o.to_s}#{o2.to_s}#{$\}")
  end


And it wrote "o->o2->nil" on the screen... But when I changed
the output
it to "123" the test failed and told me:
Expected:
  $stdout: "123"
      got:
  $stdout: "oo2->"

So I''m really confused... like I said, my breakpoint inside the print 
method never gets hit so I can''t really tell which "print"
method is
invoked...

Shay.
-- 
Posted via http://www.ruby-forum.com/.
_______________________________________________
Ironruby-core mailing list
Ironruby-core at rubyforge.org
http://rubyforge.org/mailman/listinfo/ironruby-core

I would be opposed to making the MSpec runner depend on any more stdlibs. I
would suggest making a helper that swaps the IOStub with the real stdout for the
duration of a block.

JD

....there is no try
Sent from my phone. Please excuse typos and txtspk.

-----Original Message-----
From: Shri Borde <Shri.Borde at microsoft.com>
Sent: May 16, 2009 3:11 PM
To: ironruby-core at rubyforge.org <ironruby-core at rubyforge.org>
Subject: Re: [Ironruby-core] Code Review: Fix for File.print misbehavior


The problem is because of mspec itself. In
External.LCA_RESTRICTED\Languages\IronRuby\mspec\mspec\lib\mspec\matchers\output.rb,
the code sets $stdout to a helper class (IOStub) so that it can capture the
output. However, what this means is that calls to $stdout.print that the test
makes now go to IOStub#print rather than to the real implementation of IO#print,
and this masks the bug.

A better solution would be to use StringIO in matchers\output.rb rather than
IOStub. If you are included to pursue this (which would be great), you could try
the change and/or send email to http://groups.google.com/group/rubyspec to
discuss the change. However, since this is a preexisting issue, you could leave
it for another day if you want.

So consider your change code-reviewed, and you can chose separately to pursue
(or not) the issue with matchers\output.rb.

-----Original Message-----
From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at
rubyforge.org] On Behalf Of Shay Friedman
Sent: Saturday, May 16, 2009 12:46 AM
To: ironruby-core at rubyforge.org
Subject: Re: [Ironruby-core] Code Review: Fix for File.print misbehavior

I couldn''t follow the code exactly but what I did find, was that this
test never reached the code inside IOOps.Print.
The behavior is really strange, I tried to print the value on the screen
and the value were really incorrect (the $\ delimiter is added after
each argument) but the $stdout.print call inside the lambda expression
got it correctly...

I did that:

  it "writes each obj.to_s to the stream and appends $\\ (if any) given
multiple

objects" do
    o, o2 = Object.new, Object.new
    def o.to_s(); ''o''; end
    def o2.to_s(); ''o2''; end

    puts $stdout.print(o, o2)

    lambda { $stdout.print(o, o2) }.should
output("#{o.to_s}#{o2.to_s}#{$\}")
  end


And it wrote "o->o2->nil" on the screen... But when I changed
the output
it to "123" the test failed and told me:
Expected:
  $stdout: "123"
      got:
  $stdout: "oo2->"

So I''m really confused... like I said, my breakpoint inside the print
method never gets hit so I can''t really tell which "print"
method is
invoked...

Shay.
--
Posted via http://www.ruby-forum.com/.
_______________________________________________
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

The "output" matcher from the snippet below swaps stdout with IOStub
in order to be able to capture the output of the block. Swapping IOStub back
with real stdout will not work as you will not know what the block printed...

    lambda { $stdout.print(o, o2) }.should
output("#{o.to_s}#{o2.to_s}#{$\}")

One hybrid way to fix this would be for the "output" matcher to take
an IO object as a second optional parameter that is used to capture the output
intead of using IOStub. Then print_spec.rb could provide a StringIO object.
Currently, print_spec.rb is busted and does not test what it is supposed to.
Taking a dependency on a stdlib (stringio) would not be worse thing in this
case.

I do not feel strongly about fixing this right now...
________________________________________
From: ironruby-core-bounces at rubyforge.org [ironruby-core-bounces at
rubyforge.org] On Behalf Of Jim Deville [jdeville at microsoft.com]
Sent: Saturday, May 16, 2009 4:05 PM
To: ironruby-core at rubyforge.org
Subject: Re: [Ironruby-core] Code Review: Fix for File.print misbehavior

I would be opposed to making the MSpec runner depend on any more stdlibs. I
would suggest making a helper that swaps the IOStub with the real stdout for the
duration of a block.

JD

....there is no try
Sent from my phone. Please excuse typos and txtspk.

-----Original Message-----
From: Shri Borde <Shri.Borde at microsoft.com>
Sent: May 16, 2009 3:11 PM
To: ironruby-core at rubyforge.org <ironruby-core at rubyforge.org>
Subject: Re: [Ironruby-core] Code Review: Fix for File.print misbehavior


The problem is because of mspec itself. In
External.LCA_RESTRICTED\Languages\IronRuby\mspec\mspec\lib\mspec\matchers\output.rb,
the code sets $stdout to a helper class (IOStub) so that it can capture the
output. However, what this means is that calls to $stdout.print that the test
makes now go to IOStub#print rather than to the real implementation of IO#print,
and this masks the bug.

A better solution would be to use StringIO in matchers\output.rb rather than
IOStub. If you are included to pursue this (which would be great), you could try
the change and/or send email to http://groups.google.com/group/rubyspec to
discuss the change. However, since this is a preexisting issue, you could leave
it for another day if you want.

So consider your change code-reviewed, and you can chose separately to pursue
(or not) the issue with matchers\output.rb.

-----Original Message-----
From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at
rubyforge.org] On Behalf Of Shay Friedman
Sent: Saturday, May 16, 2009 12:46 AM
To: ironruby-core at rubyforge.org
Subject: Re: [Ironruby-core] Code Review: Fix for File.print misbehavior

I couldn''t follow the code exactly but what I did find, was that this
test never reached the code inside IOOps.Print.
The behavior is really strange, I tried to print the value on the screen
and the value were really incorrect (the $\ delimiter is added after
each argument) but the $stdout.print call inside the lambda expression
got it correctly...

I did that:

  it "writes each obj.to_s to the stream and appends $\\ (if any) given
multiple

objects" do
    o, o2 = Object.new, Object.new
    def o.to_s(); ''o''; end
    def o2.to_s(); ''o2''; end

    puts $stdout.print(o, o2)

    lambda { $stdout.print(o, o2) }.should
output("#{o.to_s}#{o2.to_s}#{$\}")
  end


And it wrote "o->o2->nil" on the screen... But when I changed
the output
it to "123" the test failed and told me:
Expected:
  $stdout: "123"
      got:
  $stdout: "oo2->"

So I''m really confused... like I said, my breakpoint inside the print
method never gets hit so I can''t really tell which "print"
method is
invoked...

Shay.
--
Posted via http://www.ruby-forum.com/.
_______________________________________________
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

_______________________________________________
Ironruby-core mailing list
Ironruby-core at rubyforge.org
http://rubyforge.org/mailman/listinfo/ironruby-core