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

Improve documentation about Kernel monkeypatches #6217

Merged
merged 3 commits into from Jan 8, 2023

Conversation

nobu
Copy link
Contributor

@nobu nobu commented Dec 30, 2022

What was the end-user or developer problem that led to this PR?

https://bugs.ruby-lang.org/issues/19285

It seems we merge all of the docs for stdlib and core into one for docs.ruby-lang.org, this creates weirdness like Kernel which includes monkey patches for rubygems for example.

RubyGems adds the gem method to allow activation of specific gem versions and overrides the require method on Kernel to make gems appear as if they live on the $LOAD_PATH. See the documentation of these methods for further detail.

This seems an implementation detail but not for users.

What is your fix for the problem, implemented in this PR?

Hide this comment from RDoc.

Make sure the following tasks are checked

@deivid-rodriguez
Copy link
Member

Should we make this appear at https://docs.ruby-lang.org/en/master/Kernel.html#method-i-require instead? It does seem useful there to me.

@nobu
Copy link
Contributor Author

nobu commented Jan 1, 2023

Already lib/rubygems/core_ext/kernel_require.rb has the description, but rdoc ignores it as it is in a here doc string.

@deivid-rodriguez
Copy link
Member

Ah I see! Do you know of any workaround for that?

@nobu
Copy link
Contributor Author

nobu commented Jan 4, 2023

I came up is that:

  1. separate the code to another file
  2. read and module_eval that file
  3. and let RDoc parse the file as a normal script.

@nobu
Copy link
Contributor Author

nobu commented Jan 4, 2023

Just an idea.

diff --git i/lib/rubygems.rb w/lib/rubygems.rb
index 73a9848bf..70e51a996 100644
--- i/lib/rubygems.rb
+++ w/lib/rubygems.rb
@@ -119,10 +119,6 @@
   # to avoid deprecation warnings in Ruby 2.7.
   UNTAINT = RUBY_VERSION < "2.7" ? :untaint.to_sym : proc {}
 
-  # When https://bugs.ruby-lang.org/issues/17259 is available, there is no need to override Kernel#warn
-  KERNEL_WARN_IGNORES_INTERNAL_ENTRIES = RUBY_ENGINE == "truffleruby" ||
-                                         (RUBY_ENGINE == "ruby" && RUBY_VERSION >= "3.0")
-
   ##
   # An Array of Regexps that match windows Ruby platforms.
 
@@ -1348,7 +1344,16 @@
 Gem::Specification.load_defaults
 
 require_relative "rubygems/core_ext/kernel_gem"
-require_relative "rubygems/core_ext/kernel_require"
+
+path = File.join(__dir__, "rubygems/core_ext/kernel_require.rb")
+# When https://bugs.ruby-lang.org/issues/17259 is available, there is no need to override Kernel#warn
+if RUBY_ENGINE == "truffleruby" ||
+   (RUBY_ENGINE == "ruby" && RUBY_VERSION >= "3.0")
+  file = "<internal:#{path}>"
+else
   require_relative "rubygems/core_ext/kernel_warn"
+  file = path
+end
+module_eval File.read(path), file
 
 require ENV["BUNDLER_SETUP"] if ENV["BUNDLER_SETUP"] && !defined?(Bundler)
diff --git i/lib/rubygems/core_ext/kernel_require.rb w/lib/rubygems/core_ext/kernel_require.rb
index 8064d813e..267cfcd33 100644
--- i/lib/rubygems/core_ext/kernel_require.rb
+++ w/lib/rubygems/core_ext/kernel_require.rb
@@ -17,8 +17,6 @@
     private :gem_original_require
   end
 
-  file = Gem::KERNEL_WARN_IGNORES_INTERNAL_ENTRIES ? "<internal:#{__FILE__}>" : __FILE__
-  module_eval <<'RUBY', file, __LINE__ + 1 # rubocop:disable Style/EvalWithLocation
   ##
   # When RubyGems is required, Kernel#require is replaced with our own which
   # is capable of loading gems on demand.
@@ -168,7 +166,6 @@
       end
     end
   end
-RUBY
 
   private :require
 
diff --git i/lib/rubygems/core_ext/kernel_warn.rb w/lib/rubygems/core_ext/kernel_warn.rb
index f0f173c0b..1f4c77f04 100644
--- i/lib/rubygems/core_ext/kernel_warn.rb
+++ w/lib/rubygems/core_ext/kernel_warn.rb
@@ -1,7 +1,5 @@
 # frozen_string_literal: true
 
-if !Gem::KERNEL_WARN_IGNORES_INTERNAL_ENTRIES
-
 module Kernel
   rubygems_path = "#{__dir__}/" # Frames to be skipped start with this path.
 
@@ -50,4 +48,3 @@
     original_warn.bind(self).call(*messages, **kw)
   }
 end
-end

@nobu
Copy link
Contributor Author

nobu commented Jan 4, 2023

Also :doc: directive is needed to display require in kernel_require.rb.

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

It feels really nice, not only to have this documented at the right place, but also to no longer have our whole require redefinition wrapped in a heredoc.

Since RDoc does not parse string literals as documents, `eval` the
entire file instead of embedding in a here-document.
On the contrary, as `gem_original_require` alias is an implementation
detail but not for users, it should not be documented.
@deivid-rodriguez deivid-rodriguez changed the title Hide internal document about monkey patching Kernel Improve documentation about Kernel monkeypatches Jan 8, 2023
@deivid-rodriguez
Copy link
Member

There was no further feedback here, so I'm merging this, thanks so much @nobu!

@deivid-rodriguez deivid-rodriguez merged commit 1da658d into rubygems:master Jan 8, 2023
@nobu nobu deleted the kernel-doc branch January 8, 2023 09:52
deivid-rodriguez added a commit that referenced this pull request Jan 13, 2023
Improve documentation about `Kernel` monkeypatches

(cherry picked from commit 1da658d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants