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.

Sign up for GitHub

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
+5 −0
Open

Add PrecompiledAssets::Manifest#inspect #1

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/precompiled_assets/manifest.rb
View file
Original file line number Diff line number Diff line change
@@ -29,6 +29,11 @@ def expired?
mtime && mtime != fetch_mtime
end

def inspect
truncated_parsed_manifest = "#{@parsed_manifest.to_s[0..7]}..."
"<#{self.class.name}:#{object_id} @pathname=#{@pathname.inspect} @parsed_manifest=#{truncated_parsed_manifest}>"
Comment on lines +33 to +34
Copy link
Member

@foobear foobear Jul 26, 2023 •
edited
Loading

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.

end

private

attr_accessor :mtime