Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NoMethodError: undefined method `write' for MemFs::File:Class #20

Closed
wants to merge 2 commits into from

Conversation

rmm5t
Copy link
Contributor

@rmm5t rmm5t commented Dec 11, 2015

I know that the lack of a IO.write implementation is a known issue in the README still, but the more basic usage of it, typically written as File.write(filename, string), is something that compliments the typical File.read(filename) quite nicely.

In my test suite, for now, I'm just manually adding a MemFs::File.write method to cover the usage I was using in my project.

class MemFs::File
  # MemFs doesn't implement IO.write, because the base implementation allows
  # for file offsets and other options, but it's quite convenient to be able
  # to use File.write in its most basic use case, so let's simulate that.
  def self.write(name, string, offset = nil, open_args = {})
    File.open(name, "w") { |f| f.write(string) }
  end
end

I just wanted to open up a discussion about potentially implementing this in memfs. I understand that this is not a complete implementation. I also understand the complexities involved with fully implementing offset and open_args support.

Right now, I'm at a cross-roads, because I ran into some semi-serious troubles with FakeFS and open-uri. I decided to try MemFs. It seems to work great so far, but I ran into this File.write hurdle. I'm now considering dropping a fake filesystem from my test suite altogether, but if I decide to keep MemFs around, I'd be willing to look deeper into a full PR here.

If this isn't worth doing, feel free to close this issue, but I wanted document this, open a discussion, and share (in case any one else might benefit from this).

@simonc
Copy link
Owner

simonc commented Dec 10, 2015

Hi @rmm5t ! :)

Thank you for your interest in the project. It's definitely a thing that should be addressed. And a basic implementation that covers some options is still better than none.

If you want to give a shot at a PR I would gladly help you.

As a matter of documentation, I'll indicate here that the implementation should take place in the MemFs::IO::ClassMethods module :)


context 'when +offset+ is provided' do
it 'starts writing from the offset' do
pending("Offsets not yet implemented, because Content#write always appends.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long. [87/80]

@rmm5t
Copy link
Contributor Author

rmm5t commented Dec 11, 2015

@simonc I gave this a quick shot and converted this issue into a pull-request. Please review.

It still doesn't support offsets, but it supports the most basic case of writing a plain string to a file. e.g.:

File.write "myfile.txt", "my contents"

@rmm5t
Copy link
Contributor Author

rmm5t commented Dec 11, 2015

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

Heh. I tried to match the project's coding conventions even though the warnings from houndci better match my own preferred conventions better. Oh well. I tried.

@simonc
Copy link
Owner

simonc commented Dec 11, 2015

@rmm5t Thanks :) I will review this and we'll go forward.

Don't worry about HoundCI, I just haven't found the time to set it up according to the project. But I think I used rubocop for this one :)

@simonc
Copy link
Owner

simonc commented Dec 11, 2015

Well it seems I forgot to bind a specific version of Rubocop to the project so now it's all warnings and all. I'll have some cleanup to do in the codebase :)

@simonc
Copy link
Owner

simonc commented Dec 14, 2015

@rmm5t I think the method args handling could be lighter. By explicitly naming the arguments like the ones of IO::write it should be easier to deal with.

Would you be ok with trying with def write(path, string, offset = 0, open_args = {}) ?
Thanks a lot :)

@simonc
Copy link
Owner

simonc commented Dec 14, 2015

Oh and I try to keep the methods organized by alphabetical order. There are so many of them, it's easier to find your way :D

options = { mode: File::WRONLY, encoding: nil, open_args: nil }.merge(open_args)

if offset > 0
fail NotImplementedError, "MemFs::IO.write with offset not yet supported."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long. [84/80]

It still doesn't support offsets, but it supports the most basic case of
writing a plain string to a file. e.g.:

    File.write "myfile.txt", "my contents"
@rmm5t
Copy link
Contributor Author

rmm5t commented Dec 14, 2015

Closing and re-opening PR to trigger new Travis build. It looks like that last one borked out for bundler/rubygems reasons.

@rmm5t rmm5t closed this Dec 14, 2015
@rmm5t rmm5t reopened this Dec 14, 2015
@rmm5t
Copy link
Contributor Author

rmm5t commented Dec 14, 2015

Hrm. I don't know why the build is now failing during the bundle install step for Ruby 1.9.3 and 2.1.2, but working with Ruby 2.0.0 and 2.2.2.

@simonc
Copy link
Owner

simonc commented Dec 15, 2015

That's weird. I just pushed to master and I have the same issue, I'll look into it.

@simonc
Copy link
Owner

simonc commented Dec 18, 2015

I'm trying to fix this Travis issue. I created a branch reverting all changes since the last successful build. If it doesn't work I guess I'll try to contact them or simply change CI… I'll keep you posted.

end

file = open(path, *open_args)
file.seek(offset || 0)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not completely comfortable with this line since passing false should result in no implicit conversion from boolean (TypeError) an here it would end up being zero.

@simonc
Copy link
Owner

simonc commented Dec 21, 2015

By the way I'm in touch with Travis' support, it's a problem with the new platform they deployed. I have a workaround for know.

@rmm5t
Copy link
Contributor Author

rmm5t commented Dec 21, 2015

@simonc I added implicit type conversion support to IO.write to cover your previous concerns

@simonc simonc closed this in 9630c8c Dec 21, 2015
@simonc
Copy link
Owner

simonc commented Dec 21, 2015

Merged! Thank you! 😄

@rmm5t rmm5t deleted the io-write branch December 22, 2015 00:46
@G-Rath G-Rath mentioned this pull request Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants