Add ensure_open_and_* methods to StringIO. #2246

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

warrenseen commented Mar 29, 2013

These methods are required to allow IO.copy_stream to work against a StringIO
instance.

Alternatively, it may be better to refactor IO.copy_stream to not use these
methods, as they aren't API-compatible with the MRI IO implementation?

Add ensure_open_and_* methods to StringIO.
These methods are required to allow IO.copy_stream to work against a StringIO
instance.

Alternatively, it may be better to refactor IO.copy_stream to not use these
methods, as they aren't API-compatible with the MRI IO implementation?
Member

wilson commented Mar 30, 2013

This looks totally sane to me, given the rest of the current implementation. I also agree with you that the 'internal' protocol here could be clearer.

lib/19/stringio.rb
@@ -306,6 +309,10 @@ def pid
nil
end
+ def pipe?
+ true
+ end
@dbussink

dbussink Mar 30, 2013

Owner

Any reason this returns true and not false?

@warrenseen

warrenseen Mar 30, 2013

Contributor

Good catch. I think this actually needs to be false here. The pipe flag is only used in the StreamCopier to avoid trying to call seek on a pipe - since StringIO appears to support seek calls happily, this should be ok to change. Will fix.

@warrenseen

warrenseen Mar 31, 2013

Contributor

So looking at the MRI source, it turns out that IO.copy_stream raises an error when passed an offset when the source IO isn't backed by a file descriptor. Adjusting behaviour on this also.

@dbussink

dbussink Mar 31, 2013

Owner

Ah, so it means that the pipe? method probably can go for StringIO right?

@warrenseen

warrenseen Mar 31, 2013

Contributor

Yep, I'll be removing this and raising the same error that MRI does instead of trying to copy from an offset. (The disappointing thing is that this all works perfectly for rbx with this patch, it's just not MRI compatible behavior. sigh)

On 31/03/2013, at 9:54 PM, Dirkjan Bussink notifications@github.com wrote:

In lib/19/stringio.rb:

@@ -306,6 +309,10 @@ def pid
nil
end

  • def pipe?
  • true
  • end
    Ah, so it means that the pipe? method probably can go for StringIO right?


Reply to this email directly or view it on GitHub.

Contributor

warrenseen commented Mar 30, 2013

OK, I've just realised that the copy_stream spec doesn't fully pass on MRI 1.9 - need to fix this before merging obviously.

Contributor

warrenseen commented Apr 4, 2013

@dbussink are you able to have a look at this again. The travis failure seems to be due to a crash, specs pass for me fine.

@@ -1,5 +1,6 @@
require File.expand_path('../../../spec_helper', __FILE__)
require File.expand_path('../fixtures/classes', __FILE__)
+require 'stringio'
@dbussink

dbussink Apr 4, 2013

Owner

We don't want to include specs that depend on stuff in lib/ in spec/ruby/core. If we need specs for StringIO behavior here, they should go in spec/library/stringio

@warrenseen

warrenseen Apr 4, 2013

Contributor

sure, makes sense, although this is more about copy_stream's behaviour with an IO-ish object like stringio.

@dbussink

dbussink Apr 4, 2013

Owner

Yeah, I get that this isn't really easy / straightforward in that sense, just that lib/ dependencies inside core specs aren't something we want, especially with goals like gemifying lib etc. that can get very hairy in the future. So we are strict about that.

Owner

dbussink commented Apr 4, 2013

Could you also make sure to separate commits in spec/ruby from other commits?

Owner

dbussink commented Apr 10, 2013

Do you think you could update the pull request with the latest feedback?

Contributor

warrenseen commented Apr 11, 2013

Yep, I'll have a look at it over the next couple of days when I have some time free.

Owner

brixen commented Apr 19, 2013

I added core specs for arbitrary objects in 2bbacf1 and fixed IO.copy_stream in 0c42833. This works with StringIO.

@brixen brixen closed this Apr 19, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment