Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Method#source_code #18473

Closed
dhh opened this issue Jan 12, 2015 · 27 comments
Closed

Add Method#source_code #18473

dhh opened this issue Jan 12, 2015 · 27 comments
Assignees
Milestone

Comments

@dhh
Copy link
Member

dhh commented Jan 12, 2015

Common pattern for me is to inspect the source code of a method I'm curious about like so:

irb(main):018:0> Recording.find(904).children.method(:load).source_location
  Recording Load (0.3ms)  SELECT  `recordings`.* FROM `recordings` WHERE `recordings`.`id` = 904 LIMIT 1
=> ["/Users/david/.rbenv/versions/2.1.5/lib/ruby/gems/2.1.0/bundler/gems/rails-03e672a3daec/activerecord/lib/active_record/relation.rb", 513]

Then copy the path to the clipboard, then go to the console and do mate path, then APPLE-G to jump to the line.

Let's make that simpler:

irb(main):018:0> puts Recording.find(904).children.method(:load).source_code
  Recording Load (0.3ms)  SELECT  `recordings`.* FROM `recordings` WHERE `recordings`.`id` = 904 LIMIT 1

    # Causes the records to be loaded from the database if they have not
    # been loaded already. You can use this if for some reason you need
    # to explicitly load some records before actually using them. The
    # return value is the relation itself, not the records.
    #
    #   Post.where(published: true).load # => #<ActiveRecord::Relation>
    def load
      exec_queries unless loaded?

      self
    end
@dhh dhh added this to the 5.0.0 milestone Jan 12, 2015
@guilleiguaran guilleiguaran self-assigned this Jan 12, 2015
@guilleiguaran
Copy link
Member

guilleiguaran commented Jan 12, 2015

I'll check this!!

@sgrif
Copy link
Contributor

sgrif commented Jan 13, 2015

Doesn't pry already provide this?

@dhh
Copy link
Member Author

dhh commented Jan 13, 2015

Link?

On Jan 12, 2015, at 4:58 PM, Sean Griffin notifications@github.com wrote:

Doesn't pry already provide this?


Reply to this email directly or view it on GitHub #18473 (comment).

@sgrif
Copy link
Contributor

sgrif commented Jan 13, 2015

@dhh
Copy link
Member Author

dhh commented Jan 13, 2015

Not quite the same thing. Doesn’t seem to work off dynamic methods, like the one I used in the example.

On Jan 12, 2015, at 5:01 PM, Sean Griffin notifications@github.com wrote:

https://github.com/pry/pry/wiki/Source-browsing#Show_method https://github.com/pry/pry/wiki/Source-browsing#Show_method

Reply to this email directly or view it on GitHub #18473 (comment).

@sgrif
Copy link
Contributor

sgrif commented Jan 13, 2015

Works just fine for me?

[5] pry(main)> show-source User.first.purchases.load
  User Load (0.4ms)  SELECT  "users".* FROM "users"   ORDER BY "users"."id" ASC LIMIT 1

From: /Users/sean/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/activerecord-4.1.5/lib/active_record/relation.rb @ line 485:
Owner: ActiveRecord::Relation
Visibility: public
Number of lines: 5

def load
  exec_queries unless loaded?

  self
end

@dhh
Copy link
Member Author

dhh commented Jan 13, 2015

Ah, it wasn’t listed as one of the examples. In any case, taking on all of pry because you want this seems like shooting pigeons with canons. Pry is an omnibus of stuff.

On Jan 12, 2015, at 5:10 PM, Sean Griffin notifications@github.com wrote:

Works just fine for me?

[5] pry(main)> show-source User.first.purchases.load
User Load (0.4ms) SELECT "users".* FROM "users" ORDER BY "users"."id" ASC LIMIT 1
User Load (0.4ms) SELECT "users".* FROM "users" ORDER BY "users"."id" ASC LIMIT 1
User Load (0.3ms) SELECT "users".* FROM "users" ORDER BY "users"."id" ASC LIMIT 1
User Load (0.3ms) SELECT "users".* FROM "users" ORDER BY "users"."id" ASC LIMIT 1
User Load (0.3ms) SELECT "users".* FROM "users" ORDER BY "users"."id" ASC LIMIT 1

From: /Users/sean/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/activerecord-4.1.5/lib/active_record/relation.rb @ line 485:
Owner: ActiveRecord::Relation
Visibility: public
Number of lines: 5

def load
exec_queries unless loaded?

self
end

Reply to this email directly or view it on GitHub #18473 (comment).

@sgrif
Copy link
Contributor

sgrif commented Jan 13, 2015

It's by far the most popular debugger. Why include hefty debug methods in every rails app, when a very ubiquitous gem which focuses solely on debugging does the same thing? This feels like Not Invented Here Syndrome to me.

@dhh
Copy link
Member Author

dhh commented Jan 13, 2015

It’s great that there’s a omnibus gem that adds a ton of shit for people who want that. Rails is full of small slices of functionality that’s also included in gems with the kitchen sink included. I see no conflict with that.

On Jan 12, 2015, at 5:15 PM, Sean Griffin notifications@github.com wrote:

It's by far the most popular debugger. Why include hefty debug methods in every rails app, when a very ubiquitous gem which focuses solely on debugging does the same thing? This feels like Not Invented Here Syndrome to me.


Reply to this email directly or view it on GitHub #18473 (comment).

@sgrif
Copy link
Contributor

sgrif commented Jan 13, 2015

Just seems like we're needlessly duplicating work, and adding more code that we have to maintain. Maybe it's worth reaching out to the pry guys to see if a common module could be pulled out that just provides this functionality, rather than replicating it wholesale?

@dhh
Copy link
Member Author

dhh commented Jan 13, 2015

Please do! Not interested in taking on a dependency gem, though. But if they want to extract Method#source_code, put it into AS, and then use it from Pry, that’s cool with me 👍.

On Jan 12, 2015, at 5:19 PM, Sean Griffin notifications@github.com wrote:

Just seems like we're needlessly duplicating work, and adding more code that we have to maintain. Maybe it's worth reaching out to the pry guys to see if a common module could be pulled out that just provides this functionality, rather than replicating it wholesale?


Reply to this email directly or view it on GitHub #18473 (comment).

@sgrif
Copy link
Contributor

sgrif commented Jan 13, 2015

Can we not do this? You know it doesn't make sense for a gem which has nothing to do with Rails to move their functionality into Rails, and add AS as a dependency. I'm just saying we might want to think about the maintenance cost of adding something like this to Rails, especially when it's already provided elsewhere.

I once read a really good book by some project management software guys about how every feature you add has a hidden cost.

@dhh
Copy link
Member Author

dhh commented Jan 13, 2015

Well, how about we see what kind of work this actually turns out to be. Guillermo has already assigned the ticket to himself, so he’s interested in working on it. Let’s give him a chance to see what kind of implementation this will take.

Every feature does have a cost. That’s exactly why I have no interest in depending on Pry in Rails given their laundry list of stuff. If you want one thing out of 100 on offer, I consider it completely reasonable to reimplement that one thing, rather than depend on a package that adds the whole 100.

On Jan 12, 2015, at 5:26 PM, Sean Griffin notifications@github.com wrote:

Can we not do this? You know it doesn't make sense for a gem which has nothing to do with Rails to move their functionality into Rails, and add AS as a dependency. I'm just saying we might want to think about the maintenance cost of adding something like this to Rails, especially when it's already provided elsewhere.

I once read a really good book by some project management software guys about how every feature you add has a hidden cost.


Reply to this email directly or view it on GitHub #18473 (comment).

@sgrif
Copy link
Contributor

sgrif commented Jan 13, 2015

For the record, I'm not advocating for depending on pry in Rails. I'm advocating for having this functionality be provided by other gems, which people can choose to use if they need it.

@dhh
Copy link
Member Author

dhh commented Jan 13, 2015

So if you want to have this functionality, you have to depend on all of pry? Yeah, that's not an answer that appeals to me.

@sgrif
Copy link
Contributor

sgrif commented Jan 13, 2015

#18473 (comment)

Maybe it's worth reaching out to the pry guys to see if a common module could be pulled out that just provides this functionality, rather than replicating it wholesale?

@dhh
Copy link
Member Author

dhh commented Jan 13, 2015

If you're interested in handling that, have at it! Otherwise I'm happy to see what @guilleiguaran comes up with before considering an external dependency.

@ConradIrwin
Copy link

ConradIrwin commented Jan 13, 2015

The method_source gem is what pry uses to power this feature. I'd advise against re-implementing it, as the hard bit is working out when the method ends, particularly for dynamically defined methods when the syntax of the file needs masssaging to become ruby (see https://github.com/banister/method_source/blob/master/lib/method_source/code_helpers.rb#L20-L41).

@sgrif
Copy link
Contributor

sgrif commented Jan 13, 2015

Appears to do what we need.

irb(main):002:0> puts User.first.purchases.method(:load).source
  User Load (0.5ms)  SELECT  "users".* FROM "users"   ORDER BY "users"."id" ASC LIMIT 1
    def load
      exec_queries unless loaded?

      self
    end
=> nil

irb(main):003:0> puts User.first.purchases.method(:load).comment
  User Load (0.5ms)  SELECT  "users".* FROM "users"   ORDER BY "users"."id" ASC LIMIT 1
# Causes the records to be loaded from the database if they have not
# been loaded already. You can use this if for some reason you need
# to explicitly load some records before actually using them. The
# return value is the relation itself, not the records.
#
#   Post.where(published: true).load # => #<ActiveRecord::Relation>
=> nil

@dhh
Copy link
Member Author

dhh commented Jan 13, 2015

That could be a good temporary solution until we can just get method end
line from Ruby directly. @tenderlove, did you say someone from ruby core
was open to implementing that? Maybe even in a point release?
On Jan 12, 2015 5:55 PM, "Sean Griffin" notifications@github.com wrote:

Appears to do what we need.

irb(main):002:0> puts User.first.purchases.method(:load).source
User Load (0.5ms) SELECT "users".* FROM "users" ORDER BY "users"."id" ASC LIMIT 1
def load
exec_queries unless loaded?

  self
end

=> nil

irb(main):003:0> puts User.first.purchases.method(:load).comment
User Load (0.5ms) SELECT "users".* FROM "users" ORDER BY "users"."id" ASC LIMIT 1

Causes the records to be loaded from the database if they have not

been loaded already. You can use this if for some reason you need

to explicitly load some records before actually using them. The

return value is the relation itself, not the records.

Post.where(published: true).load # => #ActiveRecord::Relation

=> nil


Reply to this email directly or view it on GitHub
#18473 (comment).

@baweaver
Copy link
Contributor

baweaver commented Jan 13, 2015

I almost wonder if there could be a light variant of pry, stripped down much in the style of Rspec Core. There's already Pry Plus, so it wouldn't be unfathomable to further break it up. At the same time, it'd depend how @banister and @cirwin view the idea.

It may be a tinge bold to say, but Pry seems to be borderline ubiquitous. Finding a way to tie it in would be of great benefit, this granting the presence of a lighter variant.

Admittedly I find it a tinge ironic that Rails views Pry as an omnibus, but that's a different matter entirely.

@banister
Copy link

banister commented Jan 13, 2015

@baweaver i think there is potential for a gem that just contains the Pry introspection APIs. Some of our APIs go well beyond the basic functionality in method_source. But, tbh, Pry itself is a relatively small gem with only a couple of hard dependencies, so just require-ing pry and using its APIs, without starting a REPL, seems like a fairly OK solution to me.

@dhh
Copy link
Member Author

dhh commented Jan 13, 2015

I guess it depends on the circles in which you travel to see Pry as ubiquitous. I've heard the same claims about rspec: All my friends use it, so it must be The Standard.

Rails is certainly an omnibus itself, which makes it all the more imperative that we restrict our appetite for additions to be more like additional appetizers and not 21-course meals.

But always happy to consider proposals for integration. At the end of the day, we all want the same thing: Better debugging for Rails applications! If there's a good way to work with Pry to enhance that within the scope of our waist line, that would be great.

Pry has certainly put in a lot of work and thought into the debugging process and we'd be wise to learn from that 👍

On Jan 12, 2015, at 22:14, Brandon Weaver notifications@github.com wrote:

I almost wonder if there could be a light variant of pry, stripped down much in the style of Rspec Core. There's already Pry Plus, so it wouldn't be unfathomable to further break it up. At the same time, it'd depend how @banister and @cirwin view the idea.

It may be a tinge bold to say, but Pry seems to be borderline ubiquitous. Finding a way to tie it in would be of great benefit, this granting the presence of a lighter variant.

Admittedly I find it a tinge ironic that Rails views Pry as an omnibus, but that's a different matter entirely.


Reply to this email directly or view it on GitHub.

@sgrif
Copy link
Contributor

sgrif commented Jan 13, 2015

In the mean time, the feature at hand is already in a separate gem. It looks like the method_source gem which pry depends on gives exactly this feature, and nothing else. Seems to fit the bill, as well as addresses all of the concerns raised in this thread (not including an omnibus)

@dhh
Copy link
Member Author

dhh commented Jan 13, 2015

Yup. I say we start by including that, then work with Ruby core through
@tenderlove to get end definition line and make it obsolete. Maybe this can
even happen before 5.0.final and we can ship without the extra dependency.

On Tuesday, January 13, 2015, Sean Griffin notifications@github.com wrote:

In the mean time, the feature at hand is already in a separate gem. It
looks like the method_source gem which pry depends on gives exactly this
feature, and nothing else. Seems to fit the bill, as well as addresses all
of the concerns raised in this thread (not including an omnibus)


Reply to this email directly or view it on GitHub
#18473 (comment).

@sgrif
Copy link
Contributor

sgrif commented Jan 13, 2015

Rather than having it as an actual dependency of Rails, why don't we add it
to the default generated Gemfile under the development group instead.

On Tue, Jan 13, 2015, 8:30 AM David Heinemeier Hansson <
notifications@github.com> wrote:

Yup. I say we start by including that, then work with Ruby core through
@tenderlove to get end definition line and make it obsolete. Maybe this
can
even happen before 5.0.final and we can ship without the extra dependency.

On Tuesday, January 13, 2015, Sean Griffin notifications@github.com
wrote:

In the mean time, the feature at hand is already in a separate gem. It
looks like the method_source gem which pry depends on gives exactly this
feature, and nothing else. Seems to fit the bill, as well as addresses
all
of the concerns raised in this thread (not including an omnibus)


Reply to this email directly or view it on GitHub
#18473 (comment).


Reply to this email directly or view it on GitHub
#18473 (comment).

@dhh
Copy link
Member Author

dhh commented Jan 13, 2015

That sounds like a good start as well. 👍

On Jan 13, 2015, at 07:32, Sean Griffin notifications@github.com wrote:

Rather than having it as an actual dependency of Rails, why don't we add it
to the default generated Gemfile under the development group instead.

On Tue, Jan 13, 2015, 8:30 AM David Heinemeier Hansson <
notifications@github.com> wrote:

Yup. I say we start by including that, then work with Ruby core through
@tenderlove to get end definition line and make it obsolete. Maybe this
can
even happen before 5.0.final and we can ship without the extra dependency.

On Tuesday, January 13, 2015, Sean Griffin notifications@github.com
wrote:

In the mean time, the feature at hand is already in a separate gem. It
looks like the method_source gem which pry depends on gives exactly this
feature, and nothing else. Seems to fit the bill, as well as addresses
all
of the concerns raised in this thread (not including an omnibus)


Reply to this email directly or view it on GitHub
#18473 (comment).


Reply to this email directly or view it on GitHub
#18473 (comment).


Reply to this email directly or view it on GitHub.

@sgrif sgrif closed this as completed in 0b2e052 Jan 13, 2015
@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants