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

[Benjamin Fleischer]

Per @rafaelfranca I should post this here ( was https://github.com/rails/rails/issues/12411 )

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)

[Xavier Noria]

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

[Aaron Patterson]

On Mon, Sep 30, 2013 at 08:16:27PM +0200, Xavier Noria wrote:
> 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?

--
Aaron Patterson
http://tenderlovemaking.com/

[Benjamin Fleischer]

On Mon, Sep 30, 2013 at 2:09 PM, Aaron Patterson <tende...@ruby-lang.org> wrote:

On Mon, Sep 30, 2013 at 08:16:27PM +0200, Xavier Noria wrote:
> 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...

[Jeremy Kemper]

+1 to follow the #to_path coercion.

Note that RubyGems overrides Kernel.require and will need similar
attention: https://github.com/rubygems/rubygems/blob/master/lib/rubygems/core_ext/kernel_require.rb#L24-L38

> --
> You received this message because you are subscribed to the Google Groups
> "Ruby on Rails: Core" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to rubyonrails-co...@googlegroups.com.
> To post to this group, send email to rubyonra...@googlegroups.com.
> Visit this group at http://groups.google.com/group/rubyonrails-core.
> For more options, visit https://groups.google.com/groups/opt_out.

[Xavier Noria]

On Mon, Sep 30, 2013 at 9:09 PM, Aaron Patterson <tende...@ruby-lang.org> wrote:

On Mon, Sep 30, 2013 at 08:16:27PM +0200, Xavier Noria wrote:
> 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?

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.

[Xavier Noria]

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.

[Benjamin Fleischer]

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

On Mon, Sep 30, 2013 at 2:53 PM, Xavier Noria <f...@hashref.com> wrote:

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.

--
You received this message because you are subscribed to a topic in the Google Groups "Ruby on Rails: Core" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/rubyonrails-core/ESAONvNYZL4/unsubscribe.
To unsubscribe from this group and all its topics, send an email to rubyonrails-co...@googlegroups.com.

[Xavier Noria]

On Mon, Sep 30, 2013 at 10:00 PM, Benjamin Fleischer <benj...@benjaminfleischer.com> wrote:

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.

[Benjamin Fleischer]

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')

[Aaron Patterson]

:+1:

[Aaron Patterson]

On Mon, Sep 30, 2013 at 12:39:04PM -0700, Jeremy Kemper wrote:
> +1 to follow the #to_path coercion.
>
> Note that RubyGems overrides Kernel.require and will need similar
> attention: https://github.com/rubygems/rubygems/blob/master/lib/rubygems/core_ext/kernel_require.rb#L24-L38

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>