REFACTORING: supporting flag about receiving an offset or not. #722

Merged
merged 1 commit into from May 10, 2013

Projects

None yet

3 participants

@josedonizetti
Member

This is important so we can fix all the specs related to write/binwrite. I've tried to make the change in a lot of ways but all of them look very ugly. So I decided to ask feedback from you that know more the code base. @hedius @enebo

PS: write/binwrite specs need this because ruby doens't truncate the file an offset is given

@headius

Misspelled "hasOffset".

@headius headius merged commit d9a1e1d into jruby:master May 10, 2013

1 check failed

default The Travis CI build could not complete due to an error
Details
@headius
Member
headius commented May 10, 2013

I'll fix the misspelling. Thanks!

@josedonizetti josedonizetti deleted the josedonizetti:supporting_offset branch May 10, 2013
@atambo

So if I am reading this correctly this says:

If the last parameter to File.new or File.open is a boolean then it has an offset.

But when I look at the ruby docs: http://ruby-doc.org/core-2.0/IO.html#method-c-new I don't see any mention of this.

This seems to break the following rubyspecs:
https://github.com/jruby/jruby/blob/master/spec/ruby/core/file/new_spec.rb#L143
https://github.com/jruby/jruby/blob/master/spec/ruby/core/file/open_spec.rb#L483

because they pass in a boolean to File.open or File.new which doesn't really have anything to do with having an offset.

@atambo
Member
atambo commented May 11, 2013

So I think what you are trying to do is satisfy the following sentence from the docs of IO.binwrite (http://ruby-doc.org/core-1.9.3/IO.html#method-c-binwrite)

If offset is not given, the file is truncated. Otherwise, it is not truncated.

You don't need to inject a boolean as a flag into all of these methods. The way this should be done is just add:

file.openFile.setMode(file.openFile.getMode() | ModeFlags.TRUNC);

inside of the IO.binwrite method in the right spot and the ChannelDescriptor will just handle the truncation for you.

I think we should just revert this commit and retry using the approach above.

@atambo
Member
atambo commented May 11, 2013

So what I said above totally wasn't necessary it was as simple as changing the mode from "wb" to "r+w". See #726.

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