Handle TypeError from where filter gracefully
#9292
Conversation
test/test_filters.rb
Outdated
| "members" => { "name" => %w(John Jane Jimmy) }, | ||
| "roles" => %w(Admin Recruiter Manager), | ||
| } | ||
| assert_raises(TypeError) { @filter.where(hash, "name", "Jimmy") } |
There was a problem hiding this comment.
Hm, I think we want to add an assertion on the message here; this would pass without your change I think, so there's no way to know if we broke it.
lib/jekyll/filters.rb
Outdated
| "Error accessing object (#{liquid_data}) with given key. Expected an integer but " \ | ||
| "got #{property.inspect} instead" | ||
| else | ||
| "Error accessing object with key #{property}. #{e.message}" |
lib/jekyll/filters.rb
Outdated
| @@ -441,6 +441,12 @@ def read_liquid_attribute(liquid_data, property) | |||
| property.split(".").reduce(liquid_data) do |data, key| | |||
| data.respond_to?(:[]) && data[key] | |||
| end | |||
| rescue StandardError => e | |||
There was a problem hiding this comment.
Is StandardError too broad? Should we keep it to TypeError or only catch when we see the error message you mention in the PR description?
lib/jekyll/filters.rb
Outdated
| @@ -441,6 +441,14 @@ def read_liquid_attribute(liquid_data, property) | |||
| property.split(".").reduce(liquid_data) do |data, key| | |||
| data.respond_to?(:[]) && data[key] | |||
| end | |||
| rescue TypeError => e | |||
| msg = if liquid_data.is_a?(Array) | |||
| "Error accessing object (#{liquid_data}) with given key. Expected an integer but " \ | |||
There was a problem hiding this comment.
Printing the whole object could be an issue. Imagine all of site.github; it would flood the terminal output with superfluous data. How would you feel about truncating the liquid_data output?
There was a problem hiding this comment.
Ah! Good catch once again.
In theory, we could include Liquid:: StandardFilters and pass the liquid_data onto the upstream truncate filter method.
Do you think that's a good idea? Do you foresee any side-effects from this?
Update: Nevermind. I truncated directly instead.
|
@jekyllbot: merge +fix |
Ashwin Maroli: Handle TypeError from `where` filter gracefully (#9292) Merge pull request 9292
Summary
Invoking
Array#[]with a non-integer argument raisesno implicit conversion of String into Integer (TypeError)which is too technical and vague for end-users. Therefore, surface a more comprehensive message with additional context.Context
Fixes #9289
TODO
Perhaps in a future pull request: