Bug #10011: Passing a string to Pathname#relative_path_from results in NoMethodError

• Status: Closed
• Assignee: akr (Akira Tanaka)
• Target version: -
• ruby -v: ruby 2.2.0dev (2014-07-05 trunk 46706) [x86_64-darwin13]
• Backport: 2.0.0: UNKNOWN, 2.1: UNKNOWN

When a string is passed to Pathname#relative_path_from, a NoMethodError is raised.

irb(main):001:0> require "pathname"
=> true
irb(main):002:0> Pathname.new("/usr/bin/cc").relative_path_from Pathname.new("/usr/bin")
=> #<Pathname:cc>
irb(main):003:0> Pathname.new("/usr/bin/cc").relative_path_from("/usr/bin")
NoMethodError: undefined method `cleanpath' for "/usr/bin":String
	from /Users/jacknagel/.rubies/ruby-2.2.0/lib/ruby/2.2.0/pathname.rb:493:in `relative_path_from'
	from (irb):3
	from /Users/jacknagel/.rubies/ruby-2.2.0/bin/irb:11:in `<main>'

I think either converting the argument to a Pathname or raising TypeError would be acceptable here.

[leriksen (Leif Eriksen)]

ok so I have 2 possible fixes, one of which I've implemented

The unimplemented fix is maybe a new method along the lines of to_s, but for paths, called to_pathname. Then we could reopen String and define
clas String
def to_pathname
Pathname.new self
end
end

And so forth for any classes where converting to a pathname makes some semantic sense.

Then in relative_path_from can just do

base_directory.to_pathname.cleanpath.to_s at line 493, as in the above description.

But I can see that might be harder to get through than the actual, fairly naive fix I've done, which is to just convert base_directory to a Pathname if required,as per
[[https://github.com/leriksen/ruby/commit/0ab29978bb83e485784fa426340ce91df30d2d13
]]

The test code seems to imply this is probably quite acceptable.

If the commit is preferred, I'll do a pull request tomorrow unless there are issues expressed here.

Or if the :to_pathname option is preferred, I implement that instead.

I don't think we should add a method to String; there is an existing path coercion protocol (to_path and File.path) and even though String does not implement to_path, I think having a to_pathname would be confusing.

Anyway, consider concatenation:

require "pathname"
a = Pathname.new("a") # => #<Pathname:a>
a + "b" # => #<Pathname:a/b>
a + Object.new # => TypeError: no implicit conversion of Object into String

Mirroring this behavior in relative_path_from can be accomplished with a simpler patch:

diff --git a/ext/pathname/lib/pathname.rb b/ext/pathname/lib/pathname.rb
index e61aa2c..e613afd 100644
--- a/ext/pathname/lib/pathname.rb
+++ b/ext/pathname/lib/pathname.rb
@@ -490,7 +490,7 @@ def each_child(with_directory=true, &b)
   #
   def relative_path_from(base_directory)
     dest_directory = self.cleanpath.to_s
-    base_directory = base_directory.cleanpath.to_s
+    base_directory = Pathname.new(base_directory).cleanpath.to_s
     dest_prefix = dest_directory
     dest_names = []
     while r = chop_basename(dest_prefix)

[leriksen (Leif Eriksen)]

I agree with the confusion - initially I thought :to_path could be used, but despite its name it returns a string, hence the suggestion for :to_pathname, that allows things to implement a way to have themselves converted to a Pathname.

I was going to do it your way initially, but I decided on the slightly longer version because

1. I didn't want to call Pathname::new if we already had been passed a Pathname (or class that inherits from that) - I think preserving the original subclass is important
2. I wanted an nice exception, related to trying to create the Pathname, raise right at that point, to explain where the user had gone wrong.

What you have in your code is pretty much what all the tests for relative_path_from do, which might have been taken as a sign on the original implementation, however that was > 8 years ago.

Unless you (or someone else) thinks my reasons are poor, I'll commit as is in 24 hours.

At the end of the day, I just want something more friendly than a NoMethodError from inside the method, so I won't stand in the way of progress, but...

1. I didn't want to call Pathname::new if we already had been passed a Pathname (or class that inherits from that) - I think preserving the original subclass is important

I'm not sure how important that is in this case. This method only requires a Pathname so that it can call cleanpath on it, after which it is converted into a string. I guess someone may have overridden cleanpath in a subclass, but since calling it is just an implementation detail of relative_path_from I would argue that there is no expectation it will be called. Or am I missing a different reason?

2. I wanted an nice exception, related to trying to create the Pathname, raise right at that point, to explain where the user had gone wrong.

I see your point, though I do think TypeError is more appropriate. And it would be consistent with, for example:

File.rename "a", Object.new # => TypeError: no implicit conversion of Object into String

It seems ArgumentError is usually raised when there is something "wrong" with the argument (there are two examples in the body of relative_path_from) and TypeError when the incorrect type is passed.

[leriksen (Leif Eriksen)]

OK I will update to TypeError - the method argument does need to support the Pathname interface.

As for the subclass - yes that is mostly it. It's a small detail.

Thanx for the feedback.

[leriksen (Leif Eriksen)]

https://github.com/ruby/ruby/pull/666

[naruse (Yui NARUSE)]

• Status changed from Open to Assigned
• Assignee set to akr (Akira Tanaka)

[zzak (zzak _)]

• Category set to ext
• Target version set to 2.2.0

[naruse (Yui NARUSE)]

• Target version deleted (2.2.0)

[jeremyevans0 (Jeremy Evans)]

• Status changed from Assigned to Closed

This was fixed between Ruby 2.5 and 2.6:

$ ruby25 -r pathname -e 'p Pathname.new("/usr/bin/cc").relative_path_from("/usr/bin")' 
Traceback (most recent call last):
        1: from -e:1:in `<main>'
/usr/local/lib/ruby/2.5/pathname.rb:508:in `relative_path_from': undefined method `cleanpath' for "/usr/bin":String (NoMethodError)
$ ruby26 -r pathname -e 'p Pathname.new("/usr/bin/cc").relative_path_from("/usr/bin")' 
#<Pathname:cc>