Project

General

Profile

Actions

Feature #6130

closed

inspect using to_s is pain

Added by trans (Thomas Sawyer) about 12 years ago. Updated over 11 years ago.

Status:
Closed
Target version:
[ruby-core:43238]

Description

Every time I define #to_s on a class I have to remember to also redefine #inspect. It's annoying and has led me to waste hours debugging b/c the errors that can occur from it often seem strange and unrelated.

I think #inspect should have an independent definition that outputs the class name, object_id and when possible instance variable settings, and should have nothing to do with #to_s by default. We developers can always alias it as such if it is appropriate.

The only exception should be for classes that have literal representation in Ruby, such as Array, String, Hash, etc. In those cases the #inspect should give the literal representation.


Files


Related issues 1 (0 open1 closed)

Is duplicate of Ruby master - Bug #4453: Overriding #to_s changes #inspectClosedmame (Yusuke Endoh)03/01/2011Actions

Updated by mame (Yusuke Endoh) almost 12 years ago

  • Status changed from Open to Assigned
  • Assignee set to matz (Yukihiro Matsumoto)

Thomas, you meant:

class Foo
def to_s; "foo"; end
end
x = Foo.new

p "== #{ x } ==" #=> "== foo ==", as you want

p x #=> "foo", but you want: #Foo:0xXXXX

, right? If so I agree, though it may be difficult in 2.0.

--
Yusuke Endoh

Updated by Eregon (Benoit Daloze) almost 12 years ago

mame (Yusuke Endoh) wrote:

If so I agree, though it may be difficult in 2.0.

I also agree, default #inspect is nice and should not be overridden by #to_s.

If not possible for 2.0, I think it should still be considered later.

Updated by matz (Yukihiro Matsumoto) almost 12 years ago

OK, I accept. The time for it would be upto the maintainer.

Matz.

Updated by mame (Yusuke Endoh) almost 12 years ago

  • Assignee changed from matz (Yukihiro Matsumoto) to mame (Yusuke Endoh)

That's great to hear!

As 2.0 release manager, I'm positive to import this change in 2.0.

I think it does not violate 2.0 compatibility policy; the format
of a return string of #inspect is not specially specified. This
changes only the format.

However, it should be postponed to 3.0 if it affects any actual
existing products. Please let me if you know such a case.

I'd like to hear your opinion,

--
Yusuke Endoh

Updated by mame (Yusuke Endoh) almost 12 years ago

Please let me if you know such a case.
Oops. Please let me know such a case if you know.

Updated by mame (Yusuke Endoh) almost 12 years ago

I'm not sure if it is trivial to fix this issue.
Could anyone please create a patch and study the behavior?

--
Yusuke Endoh

Updated by Eregon (Benoit Daloze) almost 12 years ago

mame (Yusuke Endoh) wrote:

I'm not sure if it is trivial to fix this issue.
Could anyone please create a patch and study the behavior?

I've been working on this and wondered what to do if there is no instance variable. Should it call (dynamically) #to_s? or always use Kernel#to_s which gives the class name and pointer (which would be called anyway if #to_s was not overridden) ?

That's the first solution, use the nice #inspect output whenever there are instance variables.
Another, lighter way would be to use #to_s for internal structures (and T_DATA, so classes defined in C extensions), but never for other types (all classes defined in Ruby).

What do you think?

Updated by Eregon (Benoit Daloze) almost 12 years ago

Eregon (Benoit Daloze) wrote:

I've been working on this and wondered what to do if there is no instance variable. Should it call (dynamically) #to_s? or always use Kernel#to_s which gives the class name and pointer (which would be called anyway if #to_s was not overridden) ?

After some thoughts I believe it would not be worth changing the behavior if it was not fully consistent.
So I propose this patch.

test-all pass with only two modifications in pretty_print tests, which relied on #inspect calling #to_s.
The internal structures have been updated so #inspect is an alias of #to_s when it was not defined.
The last modification is to define #inspect on the toplevel self (only #to_s was defined).

Of course, tests for the new behavior need to be added (I'll do later if the idea is approved).

What do you think?

Updated by mame (Yusuke Endoh) almost 12 years ago

Benoit, thank you!

After some thoughts I believe it would not be worth changing the behavior if it was not fully consistent.

Agreed.

So I propose this patch.

I'll review. Please wait!

test-all pass with only two modifications in pretty_print tests, which relied on #inspect calling #to_s.

Good point. I was missing pretty_print.
It must be also changed. Akr, do you have time?

--
Yusuke Endoh

Updated by mame (Yusuke Endoh) almost 12 years ago

Hello,

So I propose this patch.

I'll review. Please wait!

Looks good to me. I'll commit it, except the change of test/test_pp.rb. Thanks!
(It surprised me that your patch also passes rubyspec; rubyspec has no test for this behavior?)

Tanaka-san, I made a patch for pp. Unless there is no objection, I'll commit this too.

diff --git a/lib/pp.rb b/lib/pp.rb
index 94269ab..6e0c797 100644
--- a/lib/pp.rb
+++ b/lib/pp.rb
@@ -265,8 +265,7 @@ class PP < PrettyPrint
module ObjectMixin
# 1. specific pretty_print
# 2. specific inspect

  • 3. specific to_s

  • 4. generic pretty_print

  • 3. generic pretty_print

    A default pretty printing method for general objects.

    It calls #pretty_print_instance_variables to list instance variables.

@@ -283,18 +282,10 @@ class PP < PrettyPrint
inspect_method = method_method.call(:inspect)
rescue NameError
end

  •  begin
    
  •    to_s_method = method_method.call(:to_s)
    
  •  rescue NameError
    
  •  end
     if inspect_method && /\(Kernel\)#/ !~ inspect_method.inspect
       q.text self.inspect
     elsif !inspect_method && self.respond_to?(:inspect)
       q.text self.inspect
    
  •  elsif to_s_method && /\(Kernel\)#/ !~ to_s_method.inspect
    
  •    q.text self.to_s
    
  •  elsif !to_s_method && self.respond_to?(:to_s)
    
  •    q.text self.to_s
     else
       q.pp_object(self)
     end
    

diff --git a/test/test_pp.rb b/test/test_pp.rb
index fe65287..acd3e83 100644
--- a/test/test_pp.rb
+++ b/test/test_pp.rb
@@ -118,7 +118,6 @@ class PPInspectTest < Test::Unit::TestCase
def a.to_s() "aaa" end
result = PP.pp(a, '')
assert_equal("#{a.inspect}\n", result)

  • assert_equal("aaa\n", result)
    end
    end

--
Yusuke Endoh

Updated by mame (Yusuke Endoh) almost 12 years ago

Oh, yeah, Benoit. I'm happy if you write tests for the new behavior :-)
Do you have a commit bit already?

--
Yusuke Endoh

Updated by Eregon (Benoit Daloze) almost 12 years ago

Hello,
mame (Yusuke Endoh) wrote:

Looks good to me. I'll commit it, except the change of test/test_pp.rb. Thanks!

Thank you for reviewing!

Oh, yeah, Benoit. I'm happy if you write tests for the new behavior :-)
Do you have a commit bit already?

No, I don't.

I'll write these tests ASAP.

(It surprised me that your patch also passes rubyspec; rubyspec has no test for this behavior?)

I'd be happy to add some tests to rubyspec too.

Updated by Eregon (Benoit Daloze) almost 12 years ago

I started writing the tests:
https://github.com/eregon/ruby/commit/f9ac4cd1daadd7c37c722c79a36296f3d161058b

I'll improve them tomorrow and write the ones for rubyspec too.

Updated by Eregon (Benoit Daloze) almost 12 years ago

I attach the updated tests (sorry about the delay :/).
Please comment is something is wrong.

After speaking with Yusuke, it seems best to not include a (complete) specification for #inspect in RubySpec yet, because the exact #inspect format might be an implementation detail.

However I'd like to add the behavior change (not calling #to_s anymore) to RubySpec after this has been merged.

Updated by drbrain (Eric Hodel) almost 12 years ago

If there are no objections I will commit this Monday (unless Benoit gets a commit bit sooner)

Updated by mame (Yusuke Endoh) over 11 years ago

I suggested giving Benoit a commit bit at the developer meeting (7/21),
and matz agreed.

Benoit, could you send your PGP public key to cvs-admin AT ruby-lang.org?
See the following procedure in detail:

http://bugs.ruby-lang.org/projects/ruby/wiki/CommitterHowto#What-to-do-for-registering-you-as-a-committer

Then, please commit your patch yourself.

I'd like to thank you for your continuing contribution!

--
Yusuke Endoh

Actions #19

Updated by Eregon (Benoit Daloze) over 11 years ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r36699.
Thomas, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


Kernel#inspect: improve consistency and do not call #to_s.

A class can now benefit from the nice default #inspect even if it
defines #to_s. Also, there is no more unexpected change in #inspect
result. Internal structures have been adapted so they don't rely
on the removed behavior (#inspect calling overridden #to_s).

  • object.c (rb_obj_inspect): Kernel#inspect: do not call #to_s.
  • test/ruby/test_object.rb (test_inspect): add tests for Kernel#inspect.
  • bignum.c, io.c, numeric.c, object.c, proc.c, vm.c (Init_*):
    alias #inspect to #to_s where it was expected.
    [ruby-core:43238][Feature #6130]

Updated by Eregon (Benoit Daloze) over 11 years ago

Hi,
mame (Yusuke Endoh) wrote:

I suggested giving Benoit a commit bit at the developer meeting (7/21),
and matz agreed.
[...]
Then, please commit your patch yourself.

I'd like to thank you for your continuing contribution!

I just committed as r36699 and I committed your PP patch as r36700.

Thank you for proposing it!

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0