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

adds spec Kernel#open is not redefined by open-uri #887

Merged
merged 1 commit into from
Oct 28, 2021

Conversation

moofkit
Copy link
Contributor

@moofkit moofkit commented Oct 25, 2021

Solving #823

Requiring 'open-uri' no longer redefines Kernel#open.
Call URI.open directly or use URI#open instead. [Misc #15893]

I'm not sure this is correct location of spec but have found other example at this place and think that they should be together.

core/kernel/open_spec.rb Outdated Show resolved Hide resolved
platform_is :windows do
out.should include("Errno::EINVAL")
out.should include("Invalid argument @ rb_sysopen - http://www.ruby-lang.org/ ")
end
Copy link
Member

Choose a reason for hiding this comment

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

I think a simpler way (if we use a subprocess anyway) would be to check this way:

before = Kernel.instance_method(:open)
require 'open-uri'
after = Kernel.instance_method(:open)
p before == after

And then the output should be "true\n".
That way we don't rely on whatever the internal error for Kernel#open is.
Could you change to that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering how to do something like that. And your proposal is very clear and simple. Thanks!

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Thank you!

@eregon eregon merged commit 54ef382 into ruby:master Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants