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

Move IO methods that rely on path to File #1800

Merged
merged 1 commit into from
Apr 9, 2018

Conversation

ggrossetie
Copy link
Member

read and each_line rely on the @path attribute (which is only define on a File class). When working with a StringIO in a Node.js environment, if Opal try to use read or each_file an exception will be thrown because @path is undefined (Opal.nil).

In this pull request, I've moved these two methods on the File class.

@iliabylich
Copy link
Contributor

iliabylich commented Apr 9, 2018

There's still a IO#read method in MRI:

$ echo hello | ruby -e "p \$stdin.read"
"hello\n"

I'm not sure why this method was initially in IO class (since it's actually a File#read method), is it a bug? If so, what happens with existing implementation (StringIO#read is there and it doesn't use any paths, but StringIO#each_line is missing). I'm totally confused 😄

EDIT: I mean, before this PR StringIO#read was valid and it's valid after, as it's implemented directly on the StringIO class, and it seems to be correct.

But before this PR StringIO#each_line was inherited from IO#each_line (and broken, as it uses @path), your PR simply removes it, right?

@ggrossetie
Copy link
Member Author

I'm not sure why this method was initially in IO class (since it's actually a File#read method), is it a bug?

Yes I think so. We did not catch this bug because at the time we were not using StringIO.read on a Node.js environment.

If so, what happens with existing implementation (StringIO#read is there and it doesn't use any paths, but StringIO#each_line is missing). I'm totally confused smile

Dan added StringIO#each_line in #1787
So we should be fine since we don't need a dedicated Node.js implementation for StringIO 😌

But before this PR StringIO#each_line was inherited from IO#each_line (and broken, as it uses @path), your PR simply removes it, right?

Actually there's two issues:

  1. The first one (resolved by this PR) is that IO#each_line and IO#read should not use @path and looking at the implementation these methods should definitely be defined in File (not in IO)
  2. The second one (might be related to our environment) is that we are calling StringIO.read on a Node.js environment where nodejs/io is loaded but Opal is calling IO.read (Node.js flavor) and thus fails (because IO.read uses path).

This PR fixes the first issue.
For the second issue I would expect Opal to call StringIO.read (defined in stdlib) but it could be the expected behavior or it could be a wrong usage in Asciidoctor.js 🤔

Does it make sense ? 😉

@iliabylich
Copy link
Contributor

@Mogztter

Dan added StringIO#each_line in #1787

My bad, I've missed the alias

Does it make sense ? 😉

Yes!

Copy link
Contributor

@iliabylich iliabylich left a comment

Choose a reason for hiding this comment

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

👍

@iliabylich iliabylich merged commit ceda559 into opal:master Apr 9, 2018
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

2 participants