Avoid eager loading in Relation#pretty_print

[BuonOmo]

Summary

We mimic the behaviour used with #inspect, only fetching up to 11
elements if records are not loaded already.

Here's the issue reproduction:

require "activerecord"

ActiveRecord::Base.establish_connection YOUR_DB_URL_THERE

ActiveRecord::Migration.new.tap do |m|
  m.create_table :foos do |t|
    t.text :name
  end
end

class Foo < ActiveRecord::Base
end

100.times { Foo.create(name: "foo#{_1}") }

pp Foo.all # loads the whole table.

Other Information

Since I'm not yet fully understanding ruby's handling of these unnammed classes (Class.new), I've checked if this was not leaking:

require "get_process_mem"

before = GetProcessMem.new.mb

10_000_000.times do |i|
  ellipsis = Class.new { def pretty_print(q); q.text("..."); end }.new
  p GetProcessMem.new.mb - before if i % 10_000 == 0
end

GC.start

p GetProcessMem.new.mb - before

And at some point memory stops growing, yet the GC does not get back all the memory. Shall I consider making it a private constant? Or just add a simple string at the end, yet it is less elegant IMHO

Also, is this relevant to AR's changelog ? I've not seen anything related to inspect or pretty_print, hence I've not added any entry

[p8]

Hey @BuonOmo, thanks for contributing to Rails!

According to the contributing guides: "you should add an entry to the top of the CHANGELOG of the framework that you modified if you're adding or removing a feature, committing a bug fix, or adding deprecation notices. Refactorings and documentation changes generally should not go to the CHANGELOG."

Changes to inspect have had Changelog entries as well:
07314e6#diff-353f4f101b1bb04938d761b6e29c88e0f147084e9e276854a77ae5d9a9a3b2a8

[BuonOmo]

@p8 I missed that one, sorry... I've added the entry and taken your suggestion 🙂

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[p8]

@BuonOmo Sorry for the late reply.
Could you rebase and add tests similar to:
https://github.com/rails/rails/blob/887ff67d5fb9787df3a0107e7286e8537f57961f/activerecord/test/cases/relations_test.rb#L1983-L1992

[BuonOmo]

@p8 all done. I've also rolled back to my previous commit: the ellipsis is now within the array rather than just after:

Before

 #<Post:0x00007f8dd431bbe0
  id: 10,
  author_id: 3,
  title: "other post by bob",
  body: "hello",
  type: "Post",
  legacy_comments_count: 0,
  taggings_with_delete_all_count: 0,
  taggings_with_destroy_count: 0,
  tags_count: 1,
  indestructible_tags_count: 0,
  tags_with_destroy_count: 0,
  tags_with_nullify_count: 0>]...

After

...
 #<Post:0x00007f8dd431bbe0
  id: 10,
  author_id: 3,
  title: "other post by bob",
  body: "hello",
  type: "Post",
  legacy_comments_count: 0,
  taggings_with_delete_all_count: 0,
  taggings_with_destroy_count: 0,
  tags_count: 1,
  indestructible_tags_count: 0,
  tags_with_destroy_count: 0,
  tags_with_nullify_count: 0>,
 ...]

[p8]

@BuonOmo Nice!
Could you group the pretty_print tests together under the inspect tests?
I think we normally don't interleave tests like this in the Rails code base.
Could you also squash the commits?

[gsamokovarov]

We'll be creating this class on every pretty print, we may as well create it once.

[BuonOmo]

@gsamokovarov

The reasoning behind creating it every time is that pp will likely be used in dev env, but not in production, hence we don't really need to define it early. I could memoize it though, but I don't think it has a point, should performance matter in pp?

Moreover, I don't know if you had the chance to read it but I've tested memory growth related to creating those classes and I doesn't seem so important. I may have overlooked something there ?

[gsamokovarov]

On a second read, do we need this whole ceremony to print the ellipsis without quotes? Why not pass the string "..." as we do in the #inspect method already?

[BuonOmo]

Fair point, it just felt weird seeing that rather than a clean ellipsis, but whatever!

[gsamokovarov]

Thank you, looks good!

[BuonOmo]

@p8 @gsamokovarov I've applied your change requests, squashed and rebased 🙂

[p8]

Thanks @BuonOmo !