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

Optimize Jekyll::Filters#item_property #7696

Merged
merged 4 commits into from Feb 6, 2020

Conversation

ashmaroli
Copy link
Member

  • This is a 🐛 bug fix.

Summary

  • property.split(".").reduce(liquid_data) only if property is a dot-notation string.
    Otherwise, property need not be split into a single-element array just to be reduced with the liquid_data object.
  • liquid_data.respond_to?(:[]) && liquid_data[key].
    Since a first-pass to :reduce can result in nil, a subsequent pass should be guarded.
    e.g. when property == "author.name" and page.author == nil
  • Minor refactoring:
    • property = property.to_s instead of multiple property.to_s
    • call #parse_sort_input at the end of method instead of placing a call at end of each conditional branch

Context

Resolves #6939
Closes #6992

@ashmaroli

This comment has been minimized.

@mattr- mattr- added this to the 4.1 milestone Jun 14, 2019
@mattr- mattr- added this to In progress in Jekyll 4.1 Jun 14, 2019
@ashmaroli
Copy link
Member Author

Profiler Summary

--- master branch https://travis-ci.org/jekyll/jekyll/jobs/567626937
+++ PR branch     https://travis-ci.org/jekyll/jekyll/jobs/571688230

- Total allocated: 485.18 MB (4549242 objects)
- Total retained:  19.16 MB (100616 objects)
+ Total allocated: 481.26 MB (4489114 objects)
+ Total retained:  19.17 MB (100692 objects)

@DirtyF DirtyF added this to In progress in Jekyll 4.1 Aug 14, 2019
@DirtyF DirtyF moved this from In progress to Reviewable in Jekyll 4.1 Aug 14, 2019
Copy link
Member

@mattr- mattr- left a comment

Choose a reason for hiding this comment

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

Looks good if we can get the test suites to pass. 😅

@DirtyF
Copy link
Member

DirtyF commented Feb 6, 2020

All green 🍏

@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit 8bb76c8 into jekyll:master Feb 6, 2020
Jekyll 4.1 automation moved this from Reviewable to Done Feb 6, 2020
jekyllbot added a commit that referenced this pull request Feb 6, 2020
@ashmaroli ashmaroli deleted the filter-item-property branch February 6, 2020 16:20
@DirtyF DirtyF moved this from In progress to Done in Jekyll 4.1 Mar 17, 2020
@jekyll jekyll locked and limited conversation to collaborators Feb 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Jekyll 4.1
  
Done
Jekyll 4.1
  
Done
Development

Successfully merging this pull request may close these issues.

Liquid where filter should fail gracefully when property does not exist
4 participants