Code Review: Fix for File.print misbehavior

Ruby IronRuby
1

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:

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

Thanks,
Shay.

2

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

3

Hi Shri,

Regarding the contributor agreement - yes, I’ve signed that (Jim D.
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.

4

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.

5

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.

6

“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