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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve Template#inspect output #35407
Conversation
Handler, formats, and variant are usually obvious from looking at the identifier. However it's not uncommon to have different locals for the same template so we should make that obvious in inspect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we print the inspect output in logs or the log subscriber? Could be good to double check what a rendering log output looks like.
@@ -202,8 +202,12 @@ def refresh(view) | |||
end | |||
end | |||
|
|||
def short_identifier | |||
@short_identifier ||= defined?(Rails.root) ? identifier.sub("#{Rails.root}/", "") : identifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I鈥檓 guessing what鈥檚 identifier and short_identifier here is what鈥檚 path(?) and virtual_path elsewhere in Action View. Might be nice if the naming was unified for our internals. Though that鈥檚 another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they are a little different. virtual_path
is relative to the view directory (ex. app/views
), where short_identifier
is relative to the app root (or from /
if from a gem outside). I think it's also possible for identifier not to be from the FS if the template comes from some other source.
I do agree that it does feel like something here could be unified though!
Gotcha! Thanks for checking both. |
This is a quality of life improvement for those developing ActionView itself (cc @tenderlove)
This PR first renames the existing
inspect
implementation toshort_identifier
. This improves the 馃憖 reading ofidentifier_method_name
, which used to readinspect.tr("^a-z_", "_")
, which is a bit awkward because it relies on whatinspect
happens to return.short_identifier.tr("^a-z_", "_")
conveys what it does better (and allows us to changeinspect
馃帀 ).Inspect used to return a string like
app/views/home/index.html.erb
, which is confusing because I would normally expect to see a type and it isn't immediately obvious that that isn't a string, particularly when in an array (ie.[some/template, another/template]
parses to me the same as["some/template", "another/template"]
on first glance).This PR changes inspect to return a string like:
Including the class name makes it look more like most
#inspect
methods. I also addedlocals
to this output because it's often different for the same identifier (we need to compile each set of locals separately). It could be helpful to include handler, formats, and variants, but they should be inferable by the developer from the identifier, so I didn't include them.I'd guess the reason we override inspect here in the first place is to avoid printing out the whole source code for the template.