Controller methods not in controller class don't warn #2061
Conversation
hannahramadan
changed the title
hannahramadan
changed the title
hannahramadan
requested review from
fallwith,
kaylareopelle and
tannalynn
as code owners
| class String | ||
| def underscore | ||
| 'test_controller' | ||
| end | ||
|
|
||
| def join(*args) | ||
| 'User/fake_app/app/controllers/test_controller.rb' | ||
| end | ||
| end |
There was a problem hiding this comment.
Cool idea! Are these methods still being used? If so, can you show me how they work?
| path = Rails.root.join("app/controllers/#{klass.name.underscore}.rb") | ||
| return [] unless File.exist?(path) | ||
|
|
||
| [name, [path.to_s, 1], is_class_method] |
There was a problem hiding this comment.
path should be a string already and not need the #to_s call
There was a problem hiding this comment.
It looks like out of the 4 method arguments, only klass is used. Maybe we should refactor this method to only return either a path or a nil, and then the caller itself can say "if I got a nil, return [] but if I got a path, return a 4 item array using the returned path and the 3 items I already have".
There was a problem hiding this comment.
When testing, the result of calling Rails.root is a Pathname. I'm not sure of all the implications of that, but because in other cases we get a String, I thought it would be safer. Can you see the downsides to it being kept as a Pathname object?
I'll work on that refactor!
There was a problem hiding this comment.
Ah,ok. Check to see if #join returns a string instead of another Pathname object. A string won't need a to_s but another Pathname will.
There was a problem hiding this comment.
Looks like .join returns another Pathname
There was a problem hiding this comment.
Perfect. Sorry for the confusion. The to_s stays.
Previously, when a new thread was spawned the agent would continue using the current transaction to record data on, even if this transaction had finished already in a different thread. Now the agent will only use the current transaction in the new thread if it is not yet finished.
hannahramadan
force-pushed
the
mlt_controller_without_method
branch
from
23bd3ac to
50a1239
Compare
SimpleCov Report
|
Co-authored-by: Kayla Reopelle (she/her) <[email protected]>
Rails controllers can be a special case because by default, controllers in Rails automatically render views with names that correspond to valid routes. This means that a controller method may not have a corresponding method in the controller class.
This PR adds a check to see if we have a valid method that isn't attached to a controller, and if so, can we figure out what controller it belongs to.
The check makes an assumption of a simple rails app with all controllers living inside app/controllers, so if a customer's app breaks this pattern, the check fails, we don't report on method metrics, and we also don't get false warnings. If we can determine the controller, the method information is inserted at the top of the file.
Community issue #2048