[Bug/Feature] require_dependency should allow Pathname, not just String Edit

Per @rafaelfranca I should post this here ( was require_dependency should allow Pathname, not just String · Issue #12411 · rails/rails · GitHub )

require_dependency should allow Pathname, not just String

Example use case

require_dependency Rails.root.join('lib','null_logger')
# raises exception
# ArgumentError: the file name must be a String -- you passed #<Pathname:/path/to/app/lib/null_logger>

I would argue require_dependency should also accept a pathname, just like require does.

I think the workaround proves this point: require_dependency Rails.root.join('lib','null_logger').to_s

underlying Rails code

def require_dependency(file_name, message = "No such file to load -- %s")
        unless file_name.is_a?(String)
          raise ArgumentError, "the file name must be a String -- you passed #{file_name.inspect}"
        end

        Dependencies.depend_on(file_name, message)
      end

I’m happy to make a PR for this if Rails core approves.

The PR would be something like

def require_dependency(file_name, message = "No such file to load -- %s")
        file_name = file_name.to_path if file_name.respond_to?(:to_path)
        unless file_name.is_a?(String)

Agreed. I believe that if the argument is not a string we should check if it responds to #to_path.

Do we even need to check? Won't it eventually get required, and Ruby will blow up?

ActiveSupport::Dependencies.depend_on(Pathname.new('foo'), 'some message')

then blows up on NoMethodError: undefined method `=~' for #<Pathname:foo> in require_or_load

so we could push that down the stack...

+1 to follow the #to_path coercion.

Note that RubyGems overrides Kernel.require and will need similar attention: rubygems/kernel_require.rb at master · rubygems/rubygems · GitHub

Delegation would be more clean, but there's quite a bit of code between the invocation and the actual Kernel#load or require call, and strings are assumed along the way (#sub, =~, etc.). It will be easier to follow the #to_path contract I believe.

Well, I see there are relatively soon calls to File.expand_path (that checks #to_path). Maybe technically a few #to_s calls here and there could make it succeed, but relying on the fact that the implementation passes through expand_path at some point seems obscure to me.

So, is current thought, then

ActiveSupport::Dependencies

def require_or_load(file_name, const_path = nil)

file_name = file_name.to_path if file_name.respond_to?(:to_path)

Kernel extension #not sure if this is necessary. vanilla ruby appears to be okay with requiring a pathname

def require path

path = path.to_path if path.respond_to?(:to_path)

RUBYGEMS_ACTIVATION_MONITOR.enter

So, is current thought, then

ActiveSupport::Dependencies

    def require_or_load(file_name, const_path = nil)

      file_name = file_name.to_path if file_name.respond_to?(:to_path)

The public API is require_dependency, require_or_load is more internal. I believe it should go into require_dependency, which is the one doing the String check nowadays.

Kernel extension #not sure if this is necessary. vanilla ruby appears to be okay with requiring a pathname

Nah, don't think so, we do not have to fix RubyGems own core extensions.

Based on discussion here, I created a PR https://github.com/rails/rails/pull/12412 with an implicit test. Let me know, there, if you’d like a more explicit one, or anything else.

def test_tracking_loaded_files require_dependency ‘dependencies/service_one’

  • require_dependency ‘dependencies/service_two’
  • require_dependency Pathname(‘dependencies/service_two’)

:+1:

Yep. I will file a bug:

irb(main):008:0> require Pathname.new('active_record') TypeError: no implicit conversion of Pathname into String         from /Users/aaron/.rbenv/versions/2.1.0-dev/lib/ruby/site_ruby/2.1.0/rubygems/core_ext/kernel_require.rb:109:in `end_with?'         from /Users/aaron/.rbenv/versions/2.1.0-dev/lib/ruby/site_ruby/2.1.0/rubygems/core_ext/kernel_require.rb:109:in `rescue in require'         from /Users/aaron/.rbenv/versions/2.1.0-dev/lib/ruby/site_ruby/2.1.0/rubygems/core_ext/kernel_require.rb:35:in `require'         from (irb):8         from /Users/aaron/.rbenv/versions/2.1.0-dev/bin/irb:11:in `<main>' irb(main):009:0>