Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Limit the number of records in Relation#inspect #6987

Merged
merged 1 commit into from Jul 7, 2012

Conversation

Projects
None yet
5 participants
Contributor

dmathieu commented Jul 6, 2012

This is a follow up of 07314e6

cc @josevalim @jonleighton

Contributor

josevalim commented Jul 6, 2012

I am not a fan of the text added, it is too long. A simple indicator (like ...) would suffice imo. Also, I don't think we should use count, since it can return hashes with group_by.

Contributor

josevalim commented Jul 6, 2012

I forgot to add but thanks for working on this <3 <3 <3

Contributor

dmathieu commented Jul 6, 2012

I changed it with an ellipsis instead of the phrase :)

@josevalim josevalim and 1 other commented on an outdated diff Jul 6, 2012

activerecord/lib/active_record/relation.rb
@@ -515,7 +515,14 @@ def values
end
def inspect
- "#<#{self.class.name} #{to_a.inspect}>"
+ if limit_value && limit_value < 10
+ entries = self
+ else
+ entries = limit(10)
@josevalim

josevalim Jul 6, 2012

Contributor

The issue is: limit(10) can return an array with 2 items. In such cases, there is no need for the elipsis.

We could limit to 11 items just to check if we need the elipsis, probably something like this:

    def inspect
      text = 
        if limit_value && limit_value <= 10
          to_a.inspect
        else
          entries = limit(11).to_a
          if entries.size > 10
            entries.pop
            entries.inspect.chop! << ", ...]"
          else
            entries.inspect
          end
        end

      "#<#{self.class.name} #{text}>"
    end
@dmathieu

dmathieu Jul 6, 2012

Contributor

Yes, my bad. I have just pushed a new version.

Limit the number of records in Relation#inspect
While it's interesting to have the results array, it can make a console or a webpage freeze if there are a lot of them.
So this limits the number of records displayed in #inspect to 10 and tells how much were effectively found.
Contributor

josevalim commented Jul 6, 2012

Looks good. @jonleighton, mmm watcha say?

Owner

rafaelfranca commented Jul 6, 2012

@dmathieu also remember to update the Release Notes

josevalim added a commit that referenced this pull request Jul 7, 2012

Merge pull request #6987 from dmathieu/limit_inspect
Limit the number of records in Relation#inspect

@josevalim josevalim merged commit 25fb6cc into rails:master Jul 7, 2012

Member

jonleighton commented Jul 7, 2012

@dmathieu thanks for working on this. I made a few more changes in 8a6780b...90e42ff

Member

jonleighton commented Jul 7, 2012

Crap, sorry! I will fix

Member

jonleighton commented Jul 7, 2012

See d8ca791 - I now changed it to not do a LIMIT at the database level, see the commit message for rationale. I'm happy to hear dissenting opinions though if others feel strongly about it...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment