implement ActiveRecord::Base#pretty_print #15172

Merged
merged 1 commit into from Jun 3, 2014

Conversation

Projects
None yet
6 participants
@notEthan
Contributor

notEthan commented May 18, 2014

AR::B has long been lacking pretty print support. I submitted this patch some years ago (pre-github), attempting again.

before:

pp topic

#<Topic id: 1, title: "The First Topic", author_name: "David", author_email_address: "david@loudthinking.com", written_on: "2003-07-16 14:28:11", bonus_time: "2000-01-01 14:28:00", last_read: "2004-04-15", content: "Have a nice day", important: nil, approved: false, replies_count: 1, unique_replies_count: 0, parent_id: nil, parent_title: nil, type: nil, group: nil, created_at: "2014-05-18 21:33:26", updated_at: "2014-05-18 21:33:26">

(same as #inspect)

now:

pp topic

#<Topic:0x007fb93696bcb0
 id: 1,
 title: "The First Topic",
 author_name: "David",
 author_email_address: "david@loudthinking.com",
 written_on: 2003-07-16 14:28:11 UTC,
 bonus_time: 2000-01-01 14:28:00 UTC,
 last_read: Thu, 15 Apr 2004,
 content: "Have a nice day",
 important: nil,
 approved: false,
 replies_count: 1,
 unique_replies_count: 0,
 parent_id: nil,
 parent_title: nil,
 type: nil,
 group: nil,
 created_at: 2014-05-18 21:31:53 UTC,
 updated_at: 2014-05-18 21:31:53 UTC>
@notEthan

This comment has been minimized.

Show comment
Hide comment
@notEthan

notEthan May 18, 2014

Contributor

here is the old submission. it lacked tests. this has tests, yay
https://rails.lighthouseapp.com/projects/8994/tickets/6498/

Contributor

notEthan commented May 18, 2014

here is the old submission. it lacked tests. this has tests, yay
https://rails.lighthouseapp.com/projects/8994/tickets/6498/

+ pp.text column_name
+ pp.text ':'
+ pp.breakable
+ pp.pp column_value

This comment has been minimized.

@rafaelfranca

rafaelfranca May 19, 2014

Member

I think we should use attribute_for_inspect here

@rafaelfranca

rafaelfranca May 19, 2014

Member

I think we should use attribute_for_inspect here

This comment has been minimized.

@notEthan

notEthan May 19, 2014

Contributor

that would cause attributes which are arrays or hashes or other serialized objects to be limited to a single line, rather than letting PP do its thing to break it over lines and indent.

@notEthan

notEthan May 19, 2014

Contributor

that would cause attributes which are arrays or hashes or other serialized objects to be limited to a single line, rather than letting PP do its thing to break it over lines and indent.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
Member

rafaelfranca commented May 19, 2014

@jeremy WDYT?

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn May 21, 2014

Member

👍

Member

arthurnn commented May 21, 2014

👍

@rafaelfranca rafaelfranca added this to the 4.2.0 milestone May 21, 2014

@notEthan

This comment has been minimized.

Show comment
Hide comment
@notEthan

notEthan May 25, 2014

Contributor

how is this looking? likely to go anywhere?

Contributor

notEthan commented May 25, 2014

how is this looking? likely to go anywhere?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 26, 2014

Member

Please add a CHANGELOG entry

Member

rafaelfranca commented May 26, 2014

Please add a CHANGELOG entry

@notEthan

This comment has been minimized.

Show comment
Hide comment
@notEthan

notEthan May 26, 2014

Contributor

added changelog and rebased

Contributor

notEthan commented May 26, 2014

added changelog and rebased

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn May 26, 2014

Member

@notEthan I guess you forgot the push -f after the rebase.

Member

arthurnn commented May 26, 2014

@notEthan I guess you forgot the push -f after the rebase.

@notEthan

This comment has been minimized.

Show comment
Hide comment
@notEthan

notEthan May 27, 2014

Contributor

no, I pushed ... 6de0d1a and 49c190c are after rebase

Contributor

notEthan commented May 27, 2014

no, I pushed ... 6de0d1a and 49c190c are after rebase

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 27, 2014

Member

Don't forget to squash your commits

Member

rafaelfranca commented May 27, 2014

Don't forget to squash your commits

@notEthan

This comment has been minimized.

Show comment
Hide comment
@notEthan

notEthan May 27, 2014

Contributor

okay, didn't know that was needed. squashed, rebased, pushed again

Contributor

notEthan commented May 27, 2014

okay, didn't know that was needed. squashed, rebased, pushed again

@notEthan

This comment has been minimized.

Show comment
Hide comment
@notEthan

notEthan May 29, 2014

Contributor

rebased again in hopes of getting this in

Contributor

notEthan commented May 29, 2014

rebased again in hopes of getting this in

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems May 29, 2014

Member

i'm 👍 on this going in. I don't regularly use pp, but when i do...i imagine this would be helpful.

Member

schneems commented May 29, 2014

i'm 👍 on this going in. I don't regularly use pp, but when i do...i imagine this would be helpful.

@rafaelfranca rafaelfranca merged commit 35983ea into rails:master Jun 3, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

rafaelfranca added a commit that referenced this pull request Jun 3, 2014

Merge pull request #15172 from notEthan/active_record_pretty_print
implement ActiveRecord::Base#pretty_print

Conflicts:
	activerecord/CHANGELOG.md
@notEthan

This comment has been minimized.

Show comment
Hide comment
@notEthan

notEthan Jun 4, 2014

Contributor

excellent. thanks @rafaelfranca

Contributor

notEthan commented Jun 4, 2014

excellent. thanks @rafaelfranca

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Jun 4, 2014

Member

❤️

Member

arthurnn commented Jun 4, 2014

❤️

@mwesigwa

This comment has been minimized.

Show comment
Hide comment
@mwesigwa

mwesigwa Jan 8, 2015

I recently submitted #18353 and have finally tracked down the cause of this issue to this commit. It turns out that while this feature is actually awesome, it also breaks some expected Ruby behavior in ActiveRecord classes. The inspect method in all ActiveRecord models is now effectively ignored in Rails 4.2 going forward.

Here is an example:

class Foo < ActiveRecord::Base
 def initialize(a=1,b=2)
    @a = a
    @b = b
  end
end
Rails(4.1) Foo.new                  #=> "#<Foo:0x0300c868 a: 1, b: 2 >"
Rails(4.2) Foo.new                  #=> 
"#<Foo:0x0300c868 
    a: 1
    b: 2>"

This is when the feature looks great and works as expected.

However consider this case:

class Foo < ActiveRecord::Base
  def initialize(a=1,b=2)
    @a = a
    @b = b
  end
  def inspect
     "Foo is awesome"
  end
end
Rails(4.1) Foo.new                  #=> "Foo is awesome"
Rails(4.2) Foo.new                  #=> 
"#<Foo:0x0300c868 
    a: 1
    b: 2>"

Here the user defined inspect method is completely ignored! Which IMO is undesirable. What do you guys think?

mwesigwa commented Jan 8, 2015

I recently submitted #18353 and have finally tracked down the cause of this issue to this commit. It turns out that while this feature is actually awesome, it also breaks some expected Ruby behavior in ActiveRecord classes. The inspect method in all ActiveRecord models is now effectively ignored in Rails 4.2 going forward.

Here is an example:

class Foo < ActiveRecord::Base
 def initialize(a=1,b=2)
    @a = a
    @b = b
  end
end
Rails(4.1) Foo.new                  #=> "#<Foo:0x0300c868 a: 1, b: 2 >"
Rails(4.2) Foo.new                  #=> 
"#<Foo:0x0300c868 
    a: 1
    b: 2>"

This is when the feature looks great and works as expected.

However consider this case:

class Foo < ActiveRecord::Base
  def initialize(a=1,b=2)
    @a = a
    @b = b
  end
  def inspect
     "Foo is awesome"
  end
end
Rails(4.1) Foo.new                  #=> "Foo is awesome"
Rails(4.2) Foo.new                  #=> 
"#<Foo:0x0300c868 
    a: 1
    b: 2>"

Here the user defined inspect method is completely ignored! Which IMO is undesirable. What do you guys think?

@notEthan

This comment has been minimized.

Show comment
Hide comment
@notEthan

notEthan Jan 8, 2015

Contributor

this appears to have been addressed as expected pry behavior (that it uses PP) over on pry/pry#1337

Contributor

notEthan commented Jan 8, 2015

this appears to have been addressed as expected pry behavior (that it uses PP) over on pry/pry#1337

@mwesigwa

This comment has been minimized.

Show comment
Hide comment
@mwesigwa

mwesigwa Jan 9, 2015

Yes, pry uses PP. But it first of all looks at the object's inspect method. Which is the expected Ruby behavior. With this change, we completely ignore the user defined inspect methods.

It seems to me that instead of defining pretty_print, you should be overriding the pp_object method which is the failsafe in Ruby in order to achieve both cases. This is my 2 cents.

And FYI, I'm the one that opened pry/pry#1337 and we concluded that this is a Rails issue.

mwesigwa commented Jan 9, 2015

Yes, pry uses PP. But it first of all looks at the object's inspect method. Which is the expected Ruby behavior. With this change, we completely ignore the user defined inspect methods.

It seems to me that instead of defining pretty_print, you should be overriding the pp_object method which is the failsafe in Ruby in order to achieve both cases. This is my 2 cents.

And FYI, I'm the one that opened pry/pry#1337 and we concluded that this is a Rails issue.

@notEthan

This comment has been minimized.

Show comment
Hide comment
@notEthan

notEthan Jan 9, 2015

Contributor

defining #pretty_print is the standard and intended way of implementing pretty-print capability. pp_object is not related to anything here.

pry uses PP. But it first of all looks at the object's inspect method

this is not correct, per the discussion at pry/pry#1337 : "PP preferentially uses pretty_print over inspect"

we concluded that this is a Rails issue

I don't see anything over there that suggests this. rf- says "If you want to customize the output for your models, you can either undef pretty_print or write your own implementation of it." which is true.

Contributor

notEthan commented Jan 9, 2015

defining #pretty_print is the standard and intended way of implementing pretty-print capability. pp_object is not related to anything here.

pry uses PP. But it first of all looks at the object's inspect method

this is not correct, per the discussion at pry/pry#1337 : "PP preferentially uses pretty_print over inspect"

we concluded that this is a Rails issue

I don't see anything over there that suggests this. rf- says "If you want to customize the output for your models, you can either undef pretty_print or write your own implementation of it." which is true.

@mwesigwa

This comment has been minimized.

Show comment
Hide comment
@mwesigwa

mwesigwa Jan 9, 2015

I think you are completely missing my point. The Ruby language uses the inspect method to "make better representation of user defined classes". It's a Ruby fundamental. The pretty_inspect method definition in Ruby itself first looks at the Object.inspect method here.

With this current implementation, you completely ignore the inspect method. This becomes a huge problem.

Consider an ActiveRecord model with 2000 attributes. If I only care about one attribute, then I would define that model's inspect method. Currently, with this implementation, you are forcing the user to see every single attribute whether they care about seeing it or not without any regard for the already defined inspect methods.

Hope this makes my point clear.

mwesigwa commented Jan 9, 2015

I think you are completely missing my point. The Ruby language uses the inspect method to "make better representation of user defined classes". It's a Ruby fundamental. The pretty_inspect method definition in Ruby itself first looks at the Object.inspect method here.

With this current implementation, you completely ignore the inspect method. This becomes a huge problem.

Consider an ActiveRecord model with 2000 attributes. If I only care about one attribute, then I would define that model's inspect method. Currently, with this implementation, you are forcing the user to see every single attribute whether they care about seeing it or not without any regard for the already defined inspect methods.

Hope this makes my point clear.

@notEthan

This comment has been minimized.

Show comment
Hide comment
@notEthan

notEthan Jan 9, 2015

Contributor

although pretty-print does fall back to inspect for objects that do not define their own pretty-printing, the implementations of inspect and pp are separate, and no other implementation of pretty printing has anything to do with #inspect (that I am aware of or have ever seen).

activerecord's pretty print implementation could in theory check if a subclass of AR::B has overridden #inspect and use that instead of its own pretty-print if so, but it seems questionable to me that this is better behavior. if PP is being used (as pry does), then a pretty-printed representation is what should be returned - not the more concise result of #inspect.

it's worth noting that this is not an issue particular to activerecord: if you subclass any other class, apart from Object, that implements pretty-printing - such as Array, Hash, many other core classes, as well as many third party libraries - pry (and anything else that uses pp) will ignore your inspect method:

[3] pry(main)> class Bar < Array
[3] pry(main)*   def inspect
[3] pry(main)*     join '+'
[3] pry(main)*   end  
[3] pry(main)* end  
=> nil
[4] pry(main)> Bar.new << 'a' << 'b'
=> ["a", "b"]
[5] pry(main)> (Bar.new << 'a' << 'b').inspect
=> "a+b"
Contributor

notEthan commented Jan 9, 2015

although pretty-print does fall back to inspect for objects that do not define their own pretty-printing, the implementations of inspect and pp are separate, and no other implementation of pretty printing has anything to do with #inspect (that I am aware of or have ever seen).

activerecord's pretty print implementation could in theory check if a subclass of AR::B has overridden #inspect and use that instead of its own pretty-print if so, but it seems questionable to me that this is better behavior. if PP is being used (as pry does), then a pretty-printed representation is what should be returned - not the more concise result of #inspect.

it's worth noting that this is not an issue particular to activerecord: if you subclass any other class, apart from Object, that implements pretty-printing - such as Array, Hash, many other core classes, as well as many third party libraries - pry (and anything else that uses pp) will ignore your inspect method:

[3] pry(main)> class Bar < Array
[3] pry(main)*   def inspect
[3] pry(main)*     join '+'
[3] pry(main)*   end  
[3] pry(main)* end  
=> nil
[4] pry(main)> Bar.new << 'a' << 'b'
=> ["a", "b"]
[5] pry(main)> (Bar.new << 'a' << 'b').inspect
=> "a+b"
@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Jan 10, 2015

Member

if PP is being used (as pry does), then a pretty-printed representation is what should be returned - not the more concise result of #inspect

FWIW, I think I disagree: I can't think of any instance where PP intends to be more informative than the corresponding inspect -- only better formatted.

I'd thus consider the custom-inspect-on-Array-subclass behaviour you demonstrated to be an unfortunate shortcoming in pry's logic.

activerecord's pretty print implementation could in theory check if a subclass of AR::B has overridden #inspect

I think we should do this: enhance our own inspect's presentation, but not fight with a custom one. IMO, this seems more likely to be what more people are going to expect -- and visually consistent with our previous behaviour.

Member

matthewd commented Jan 10, 2015

if PP is being used (as pry does), then a pretty-printed representation is what should be returned - not the more concise result of #inspect

FWIW, I think I disagree: I can't think of any instance where PP intends to be more informative than the corresponding inspect -- only better formatted.

I'd thus consider the custom-inspect-on-Array-subclass behaviour you demonstrated to be an unfortunate shortcoming in pry's logic.

activerecord's pretty print implementation could in theory check if a subclass of AR::B has overridden #inspect

I think we should do this: enhance our own inspect's presentation, but not fight with a custom one. IMO, this seems more likely to be what more people are going to expect -- and visually consistent with our previous behaviour.

@notEthan

This comment has been minimized.

Show comment
Hide comment
@notEthan

notEthan Jan 10, 2015

Contributor

FWIW, I think I disagree: I can't think of any instance where PP intends to be more informative than the corresponding inspect -- only better formatted.

true

if that is the way to go, it looks like it is simple enough to implement

  def pretty_print(pp)
    if self.class.instance_method(:inspect).owner < ActiveRecord::Base
      pp.text inspect
    else
      [current implementation]
    end
  end
Contributor

notEthan commented Jan 10, 2015

FWIW, I think I disagree: I can't think of any instance where PP intends to be more informative than the corresponding inspect -- only better formatted.

true

if that is the way to go, it looks like it is simple enough to implement

  def pretty_print(pp)
    if self.class.instance_method(:inspect).owner < ActiveRecord::Base
      pp.text inspect
    else
      [current implementation]
    end
  end
@notEthan

This comment has been minimized.

Show comment
Hide comment
@notEthan

notEthan Jan 10, 2015

Contributor

hm, that won't work if an included module of a subclass redefines inspect. maybe

self.class.instance_method(:inspect).owner != ActiveRecord::Base.instance_method(:inspect).owner
Contributor

notEthan commented Jan 10, 2015

hm, that won't work if an included module of a subclass redefines inspect. maybe

self.class.instance_method(:inspect).owner != ActiveRecord::Base.instance_method(:inspect).owner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment