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 PrecompiledAssets::Manifest#inspect #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

NiklasHae
Copy link
Member

Sometimes an exception leads to printing the inspected manifest object. This fills multiple screens with the parsed manifest hash. Most of the times though, the exception's message Could not find "foobar.css" in manifest: <PrecompiledAssets::Manifest ....> already helps to solve the problem.

Instead of relying on the #inpect default implementation it truncates @parsed_manifest.

Feel free to decline without comment if you don't want this change merged ;) Thx 👍

Instead of relying on the #inpect default implementation it truncates @parsed_manifest (a potenitally huge hash filling many terminal windows with characters)
Comment on lines +33 to +34
truncated_parsed_manifest = "#{@parsed_manifest.to_s[0..7]}..."
"<#{self.class.name}:#{object_id} @pathname=#{@pathname.inspect} @parsed_manifest=#{truncated_parsed_manifest}>"
Copy link
Member

@foobear foobear Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your suggestion. A useful inspection does make sense.
However, I think there is room for improvement here.

  • Convention for object inspection is to start with #<, not <#.
  • If the manifest has not yet been parsed this inspects with @parsed_manifest=... instead of @pared_manifest=....
  • I'm not sure how helpful the first 8 characters of the manifest.json can be. Maybe we should show more characters, or offer some kind of aggregation, like @parsed_manifest=(23 entries).
  • The object_id is not actually put directly into inspection by default, but implementation varies across Ruby versions, so maybe don't want to go there. I'm fine with keeping it as implemented, or not displaying it at all.

With a few adjustments, we can get an inspection like

#<PrecompiledAssets::Manifest:12345 @pathname=#<Pathname:/path/to/manifest.json> @parsed_manifest={"application.js"=>"application-5ZACKM24...>

or, if the manifest has not been parsed,

#<PrecompiledAssets::Manifest:12345 @pathname=#<Pathname:/path/to/manifest.json> @parsed_manifest=nil>
Suggested change
truncated_parsed_manifest = "#{@parsed_manifest.to_s[0..7]}..."
"<#{self.class.name}:#{object_id} @pathname=#{@pathname.inspect} @parsed_manifest=#{truncated_parsed_manifest}>"
manifest_inspection = @parsed_manifest.inspect.sub(/\A(.{40}).+\z/, '\1...')
"#<{self.class.name}:#{object_id} @pathname=#{@pathname.inspect} @parsed_manifest=#{manifest_inspection}>"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Good catch
  • 👍
  • I like the @parsed_manifest=(23 entries) suggestion 👍
  • I've been here https://bugs.ruby-lang.org/issues/17199 before, and I just used object_id :D. A little bit weird for Ruby to not have a straightforward way to override #inspect.

Since I'm running a patched version anyways, I'll look into adapting this PR with your suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants