Rails core - Sep 2013 - [Bug/Feature] require_dependency should allow Pathname, not just String Edit

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<https://github.com/rails/rails/blob/master/activesupport/lib/active_support/dependencies.rb#L201>

      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)

-- 
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-core+unsubscribe@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/groups/opt_out.

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

-- 
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-core+unsubscribe@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/groups/opt_out.

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/

-- 
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-core+unsubscribe@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/groups/opt_out.

On Mon, Sep 30, 2013 at 2:09 PM, Aaron Patterson
<tenderlove@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...

-- 
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-core+unsubscribe@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/groups/opt_out.

+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

On Mon, Sep 30, 2013 at 11:00 AM, Benjamin Fleischer
<bfleischer@gmail.com> wrote:
> 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)
>
> --
> 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-core+unsubscribe@googlegroups.com.
> To post to this group, send email to rubyonrails-core@googlegroups.com.
> Visit this group at http://groups.google.com/group/rubyonrails-core.
> For more options, visit https://groups.google.com/groups/opt_out.
-- 
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-core+unsubscribe@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/groups/opt_out.

On Mon, Sep 30, 2013 at 9:09 PM, Aaron Patterson
<tenderlove@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.

-- 
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-core+unsubscribe@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/groups/opt_out.

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 the Google Groups
"Ruby on Rails: Core" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to rubyonrails-core+unsubscribe@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/groups/opt_out.

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 <fxn@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-core+unsubscribe@googlegroups.com.
> To post to this group, send email to rubyonrails-core@googlegroups.com.
> Visit this group at http://groups.google.com/group/rubyonrails-core.
> For more options, visit https://groups.google.com/groups/opt_out.
>
-- 
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-core+unsubscribe@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/groups/opt_out.

On Mon, Sep 30, 2013 at 10:00 PM, Benjamin Fleischer <
benjamin@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.

-- 
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-core+unsubscribe@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/groups/opt_out.

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


On Mon, Sep 30, 2013 at 3:12 PM, Xavier Noria <fxn@hashref.com> wrote:
> On Mon, Sep 30, 2013 at 10:00 PM, Benjamin Fleischer <
> benjamin@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.
>
>  --
> 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-core+unsubscribe@googlegroups.com.
> To post to this group, send email to rubyonrails-core@googlegroups.com.
> Visit this group at http://groups.google.com/group/rubyonrails-core.
> For more options, visit https://groups.google.com/groups/opt_out.
>
-- 
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-core+unsubscribe@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/groups/opt_out.

On Mon, Sep 30, 2013 at 09:44:42PM +0200, Xavier Noria
wrote:
> On Mon, Sep 30, 2013 at 9:09 PM, Aaron Patterson
> <tenderlove@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.
:+1:

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

-- 
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-core+unsubscribe@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/groups/opt_out.

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>

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

-- 
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-core+unsubscribe@googlegroups.com.
To post to this group, send email to rubyonrails-core@googlegroups.com.
Visit this group at http://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/groups/opt_out.