Add a where_exp filter for filtering by expression #4478

Merged
merged 3 commits into from Apr 13, 2016

Conversation

Projects
None yet
@IgnoredAmbience
Contributor

IgnoredAmbience commented Feb 5, 2016

This commit introduces a where_exp filter, which can be used as follows:
{{ array | where_exp: "item", "item == 10" }}
{{ array | where_exp: "item", "item.field > 10" }}
{{ site.posts | where_exp: "post", "post.keys contains 'field'" }}
{{ site.posts | where_exp: "post", "post.array contains 'giraffes'" }}

This permits a variety of use cases, such as reported in: jekyll#4467,
jekyll#4385, jekyll#2787.

@parkr parkr added the feature label Feb 9, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 23, 2016

Member

I'm 👍 on this as a feature. I think this adds power to Jekyll without making it too complicated, as it's extending current functionality. I'd like to rename where_exp to something more user-friendly. Also wondering if where could do this:

{{ array | where: "item", "item == 10" }}
{{ array | where: "field", "10", ">" }}
{{ site.posts | where: "keys", "field", "contains" }}
{{ site.posts | where: "array", "giraffes", "contains" }}

Postfix is hardly ever used, though, so I think I like yours better.

/cc @jekyll/documentation for documentation review.
/cc @jekyll/stability to help with the name for this filter. Perhaps matches?

Member

parkr commented Mar 23, 2016

I'm 👍 on this as a feature. I think this adds power to Jekyll without making it too complicated, as it's extending current functionality. I'd like to rename where_exp to something more user-friendly. Also wondering if where could do this:

{{ array | where: "item", "item == 10" }}
{{ array | where: "field", "10", ">" }}
{{ site.posts | where: "keys", "field", "contains" }}
{{ site.posts | where: "array", "giraffes", "contains" }}

Postfix is hardly ever used, though, so I think I like yours better.

/cc @jekyll/documentation for documentation review.
/cc @jekyll/stability to help with the name for this filter. Perhaps matches?

@parkr parkr added this to the flexible milestone Mar 23, 2016

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Mar 24, 2016

Contributor

A meta-question, that shouldn't derail the discussion here, but should we be adding non-Jekyll-specific liquid filters to Jekyll? Shouldn't we instead try to get those adopted upstream in Liquid itself (and its fancy new documentation)? Would they accept something like this? If not, why not?

Contributor

benbalter commented Mar 24, 2016

A meta-question, that shouldn't derail the discussion here, but should we be adding non-Jekyll-specific liquid filters to Jekyll? Shouldn't we instead try to get those adopted upstream in Liquid itself (and its fancy new documentation)? Would they accept something like this? If not, why not?

site/_docs/templates.md
+ <code class="filter">{% raw %}{{ site.members | where_exp:"item",
+"item.graduation_year < 2014" }}{% endraw %}</code>
+ <code class="filter">{% raw %}{{ site.members | where_exp:"item",
+"item.projects includes 'foo'" }}{% endraw %}</code>

This comment has been minimized.

@mneumegen

mneumegen Mar 24, 2016

Contributor

Should this be "item.projects contains 'foo'?

@mneumegen

mneumegen Mar 24, 2016

Contributor

Should this be "item.projects contains 'foo'?

This comment has been minimized.

@IgnoredAmbience

IgnoredAmbience Mar 24, 2016

Contributor

Good catch. I have a feeling I need to write more test cases for the contains case also as I tripped myself up using it in production.

@IgnoredAmbience

IgnoredAmbience Mar 24, 2016

Contributor

Good catch. I have a feeling I need to write more test cases for the contains case also as I tripped myself up using it in production.

@mneumegen

This comment has been minimized.

Show comment
Hide comment
@mneumegen

mneumegen Mar 24, 2016

Contributor

Love this feature. I'm not a big fan of the postfix notation, I find it much harder to read.

Contributor

mneumegen commented Mar 24, 2016

Love this feature. I'm not a big fan of the postfix notation, I find it much harder to read.

@IgnoredAmbience

This comment has been minimized.

Show comment
Hide comment
@IgnoredAmbience

IgnoredAmbience Mar 24, 2016

Contributor

@benbalter Since this relies on liquid internals(?), the best place probably is upstream. However, this was in direct response to a feature request here (#4385) though, so I thought it could be of immediate use to the original reporter.

Contributor

IgnoredAmbience commented Mar 24, 2016

@benbalter Since this relies on liquid internals(?), the best place probably is upstream. However, this was in direct response to a feature request here (#4385) though, so I thought it could be of immediate use to the original reporter.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 24, 2016

Member

should we be adding non-Jekyll-specific liquid filters to Jekyll? Shouldn't we instead try to get those adopted upstream in Liquid itself

@benbalter We have a much stronger case to make later if we can say "Look how this is used in Jekyll – wouldn't it be super cool if you could use them everywhere there is Liquid?" The bar for adding new features to Liquid core is fairly high as it impacts all themes on Shopify.com. It's worth submitting, but as Liquid is on v4 already ( #4362 ), any new releases there would still require a Jekyll bump. That, combined with the fact that this is using Liquid 3 internals may cause problems.

@edgemaster I wonder if it's possible to unify on one variable name, like item. It's a bit confusing for new users, though, so that's why I'm OK with it as-is (verbosity can sometimes be really helpful). Would you please add more test cases for contains as noted and fix up the docs? It looks solid to me but any added assurance is good by me.

Member

parkr commented Mar 24, 2016

should we be adding non-Jekyll-specific liquid filters to Jekyll? Shouldn't we instead try to get those adopted upstream in Liquid itself

@benbalter We have a much stronger case to make later if we can say "Look how this is used in Jekyll – wouldn't it be super cool if you could use them everywhere there is Liquid?" The bar for adding new features to Liquid core is fairly high as it impacts all themes on Shopify.com. It's worth submitting, but as Liquid is on v4 already ( #4362 ), any new releases there would still require a Jekyll bump. That, combined with the fact that this is using Liquid 3 internals may cause problems.

@edgemaster I wonder if it's possible to unify on one variable name, like item. It's a bit confusing for new users, though, so that's why I'm OK with it as-is (verbosity can sometimes be really helpful). Would you please add more test cases for contains as noted and fix up the docs? It looks solid to me but any added assurance is good by me.

lib/jekyll/filters.rb
+ # Returns the filtered array of objects
+ def where_exp(input, variable, expression)
+ return input unless input.is_a?(Enumerable)
+ input = input.values if input.is_a?(Hash)

This comment has been minimized.

@envygeeks

envygeeks Mar 25, 2016

Contributor

This will cache many off-guard. I disagree with this.

@envygeeks

envygeeks Mar 25, 2016

Contributor

This will cache many off-guard. I disagree with this.

This comment has been minimized.

@IgnoredAmbience

IgnoredAmbience Apr 12, 2016

Contributor

My motivation was to follow the existing semantics of where over hashes as in

input = input.values if input.is_a?(Hash)

@IgnoredAmbience

IgnoredAmbience Apr 12, 2016

Contributor

My motivation was to follow the existing semantics of where over hashes as in

input = input.values if input.is_a?(Hash)

lib/jekyll/filters.rb
+ # Parse a string to a Liquid Condition
+ def parse_condition(exp)
+ parser = Liquid::Parser.new(exp)
+ a = parser.expression

This comment has been minimized.

@envygeeks

envygeeks Mar 25, 2016

Contributor

Please make these variable names mean something.

@envygeeks

envygeeks Mar 25, 2016

Contributor

Please make these variable names mean something.

lib/jekyll/filters.rb
+ parser = Liquid::Parser.new(exp)
+ a = parser.expression
+ if op = parser.consume?(:comparison)
+ b = parser.expression

This comment has been minimized.

@envygeeks

envygeeks Mar 25, 2016

Contributor

""

@envygeeks

envygeeks Mar 25, 2016

Contributor

""

lib/jekyll/filters.rb
+ a = parser.expression
+ if op = parser.consume?(:comparison)
+ b = parser.expression
+ condition = Liquid::Condition.new(a, op, b)

This comment has been minimized.

@envygeeks

envygeeks Mar 25, 2016

Contributor

Please do:

var = \
if condition
  val1
else
  val2
end

Or for bonus points and extreme readability:

var = begin
  if condition
    val1
  else
    val2
  end
end
@envygeeks

envygeeks Mar 25, 2016

Contributor

Please do:

var = \
if condition
  val1
else
  val2
end

Or for bonus points and extreme readability:

var = begin
  if condition
    val1
  else
    val2
  end
end

This comment has been minimized.

@parkr

parkr Mar 25, 2016

Member

Hmm, is that in our Rubocop setup? I don't like either of those personally – I find them both harder to read than the current version. Indentation in the first example doesn't help me know that the if statement affects var and the second one is noisy. I think with better variable names (not just a, b, and op), we can make this code more readable.

@parkr

parkr Mar 25, 2016

Member

Hmm, is that in our Rubocop setup? I don't like either of those personally – I find them both harder to read than the current version. Indentation in the first example doesn't help me know that the if statement affects var and the second one is noisy. I think with better variable names (not just a, b, and op), we can make this code more readable.

This comment has been minimized.

@envygeeks

envygeeks Mar 25, 2016

Contributor

Our Rubocop setup is setup to accept any style as long as it's not multiple defines. So if you like to indent on the var like:

var = \
  if blah
    var
  else
    var
  end

It'll accept it. Those two are just recommendations, the former can be added.

@envygeeks

envygeeks Mar 25, 2016

Contributor

Our Rubocop setup is setup to accept any style as long as it's not multiple defines. So if you like to indent on the var like:

var = \
  if blah
    var
  else
    var
  end

It'll accept it. Those two are just recommendations, the former can be added.

IgnoredAmbience added some commits Feb 5, 2016

Add a where_exp filter for filtering by expression
This commit introduces a where_exp filter, which can be used as follows:
  `{{ array | where_exp: "item", "item == 10" }}`
  `{{ array | where_exp: "item", "item.field > 10" }}`
  `{{ site.posts | where_exp: "post", "post contains 'field'" }}`
  `{{ site.posts | where_exp: "post", "post.array contains 'giraffes'" }}`

This permits a variety of use cases, such as reported in: jekyll#4467,
jekyll#4385, jekyll#2787.
@IgnoredAmbience

This comment has been minimized.

Show comment
Hide comment
@IgnoredAmbience

IgnoredAmbience Apr 12, 2016

Contributor

Made the minor adjustments as suggested, except for #4478 (comment), I have chosen to filter over the values of the hash to match how the existing where filter behaves. (In fact, filtering over the keys of the input hash probably wouldn't be the intended operation for the end-user, anyway).

Contributor

IgnoredAmbience commented Apr 12, 2016

Made the minor adjustments as suggested, except for #4478 (comment), I have chosen to filter over the values of the hash to match how the existing where filter behaves. (In fact, filtering over the keys of the input hash probably wouldn't be the intended operation for the end-user, anyway).

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 13, 2016

Member

Ooooh, people are going to love this feature. 👍 ❤️

@jekyllbot: merge +minor

Member

parkr commented Apr 13, 2016

Ooooh, people are going to love this feature. 👍 ❤️

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit fc0d440 into jekyll:master Apr 13, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@parkr parkr modified the milestones: 3.2, flexible Apr 13, 2016

jekyllbot added a commit that referenced this pull request Apr 13, 2016

@IgnoredAmbience

This comment has been minimized.

Show comment
Hide comment
@IgnoredAmbience

IgnoredAmbience Apr 14, 2016

Contributor

Oh, wow. I wasn't expecting this to be landed without the more major changes (eg, to support a 3-argument where filter) suggested by @parkr

Contributor

IgnoredAmbience commented Apr 14, 2016

Oh, wow. I wasn't expecting this to be landed without the more major changes (eg, to support a 3-argument where filter) suggested by @parkr

parkr added a commit that referenced this pull request Apr 21, 2016

Merge branch 'master' into themes
* master: (58 commits)
  Update history to reflect merge of #4792 [ci skip]
  Update history to reflect merge of #4793 [ci skip]
  Update history to reflect merge of #4804 [ci skip]
  Update history to reflect merge of #4754 [ci skip]
  Update history to reflect merge of #4813 [ci skip]
  Added missing single quote on rsync client side command
  Add v3.0.4 and v3.1.3 to the history.
  Fixed typo
  Add jekyll-autoprefixer plugin
  Explicitly require Filters rather than implicitly.
  Update history to reflect merge of #4786 [ci skip]
  Update history to reflect merge of #4789 [ci skip]
  updates example domain in config template
  Globalize Jekyll's Filters.
  Update JRuby to 9.0.5.0; Drop the double digit test.
  Update Rack-Jekyll Heroku deployment blog post url
  convertible: use Document::YAML_FRONT_MATTER_REGEXP to parse transformable files
  Update history to reflect merge of #4734 [ci skip]
  Update history to reflect merge of #4478 [ci skip]
  Fix rubocop warning.
  ...
@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Apr 26, 2016

Member

This doesn't seem to work with site.posts at all, unless I am missing something…?

Member

pathawks commented Apr 26, 2016

This doesn't seem to work with site.posts at all, unless I am missing something…?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 27, 2016

Member

@pathawks Can you provide an example of something you're unable to accomplish? Perhaps add a test and submit a PR?

Member

parkr commented Apr 27, 2016

@pathawks Can you provide an example of something you're unable to accomplish? Perhaps add a test and submit a PR?

@budparr

This comment has been minimized.

Show comment
Hide comment
@budparr

budparr Apr 27, 2016

Contributor

Is this in 3.1x? I see the 3.2 milestone, but it's in the docs already, so I assume it's available now. https://jekyllrb.com/docs/templates/#filters

Contributor

budparr commented Apr 27, 2016

Is this in 3.1x? I see the 3.2 milestone, but it's in the docs already, so I assume it's available now. https://jekyllrb.com/docs/templates/#filters

@IgnoredAmbience

This comment has been minimized.

Show comment
Hide comment
@IgnoredAmbience

IgnoredAmbience Apr 28, 2016

Contributor

It doesn't appear to have landed in v3.1.3.

Contributor

IgnoredAmbience commented Apr 28, 2016

It doesn't appear to have landed in v3.1.3.

@IgnoredAmbience

This comment has been minimized.

Show comment
Hide comment
@IgnoredAmbience

IgnoredAmbience Apr 28, 2016

Contributor

With regards to it not working with site.posts, would I be correct in thinking that the appropriate place to add a test for this would be in features/embed_filters.feature?

Contributor

IgnoredAmbience commented Apr 28, 2016

With regards to it not working with site.posts, would I be correct in thinking that the appropriate place to add a test for this would be in features/embed_filters.feature?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 28, 2016

Contributor

@edgemaster no, it's a unit problem, not a feature problem. Your feature should define a high level set of behaviors, your units should mutate those behaviors to ensure your code is up to snuff.

Contributor

envygeeks commented Apr 28, 2016

@edgemaster no, it's a unit problem, not a feature problem. Your feature should define a high level set of behaviors, your units should mutate those behaviors to ensure your code is up to snuff.

@ryrych

This comment has been minimized.

Show comment
Hide comment
@ryrych

ryrych Apr 30, 2016

Hi, is there any alternative to where_exp till the feature is released / supported on Github pages? I'd like to filter out posts which contain protip path. I can add another property to a post like protip: true and iterate over the collection: {% assign posts = site.posts | where: "protip", "false" %}. The problem is that it doesn't go well with pagination.

Yeah, sorry if the question was better asked for on StackOverflow.

ryrych commented Apr 30, 2016

Hi, is there any alternative to where_exp till the feature is released / supported on Github pages? I'd like to filter out posts which contain protip path. I can add another property to a post like protip: true and iterate over the collection: {% assign posts = site.posts | where: "protip", "false" %}. The problem is that it doesn't go well with pagination.

Yeah, sorry if the question was better asked for on StackOverflow.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 12, 2016

Member

Hi, is there any alternative to where_exp till the feature is released / supported on Github pages?

@ryrych Not directly – what this filter does is collapse:

{% assign things = "" | split: "|" %}
{% for item in my_array %}
{% if <EXPR> %}
  {% assign things = things | push: item %}
{% endif %}
{% endfor %}

into

{% assign things = my_array | where_exp: "item", "<EXPR>" %} 

This will have to wait until GitHub Pages upgrades to 3.2 to use on GitHub Pages.

Member

parkr commented May 12, 2016

Hi, is there any alternative to where_exp till the feature is released / supported on Github pages?

@ryrych Not directly – what this filter does is collapse:

{% assign things = "" | split: "|" %}
{% for item in my_array %}
{% if <EXPR> %}
  {% assign things = things | push: item %}
{% endif %}
{% endfor %}

into

{% assign things = my_array | where_exp: "item", "<EXPR>" %} 

This will have to wait until GitHub Pages upgrades to 3.2 to use on GitHub Pages.

@WasabiFan WasabiFan referenced this pull request in ev3dev/ev3dev.github.io Aug 3, 2016

Merged

Limit atom feed to 10 posts #192

@TWiStErRob

This comment has been minimized.

Show comment
Hide comment
@TWiStErRob

TWiStErRob Sep 21, 2016

Contributor

What is the language of the expression? I guess it's Liquid and not Ruby.

Contributor

TWiStErRob commented Sep 21, 2016

What is the language of the expression? I guess it's Liquid and not Ruby.

@su-narthur

This comment has been minimized.

Show comment
Hide comment
@su-narthur

su-narthur Feb 20, 2017

This is a pretty cool feature! The thing it doesn't address, though are expressions that reference other variables:

my_page.permalink contains section.permalink

I guess that's probably too much complexity to cram into a filter.

This is a pretty cool feature! The thing it doesn't address, though are expressions that reference other variables:

my_page.permalink contains section.permalink

I guess that's probably too much complexity to cram into a filter.

@mryellow

This comment has been minimized.

Show comment
Hide comment
@mryellow

mryellow Mar 7, 2017

#4478 (comment)

(In fact, filtering over the keys of the input hash probably wouldn't be the intended operation for the end-user, anyway).

Unfortunately that is my use-case and this seems to come up lacking. I have a hash with epochs as keys which needs to be filtered for begin/finish dates.

Filtering with where_exp:"item", "item.key > 0" heads in the right direction but results in only the value being output rather than the key and value.

mryellow commented Mar 7, 2017

#4478 (comment)

(In fact, filtering over the keys of the input hash probably wouldn't be the intended operation for the end-user, anyway).

Unfortunately that is my use-case and this seems to come up lacking. I have a hash with epochs as keys which needs to be filtered for begin/finish dates.

Filtering with where_exp:"item", "item.key > 0" heads in the right direction but results in only the value being output rather than the key and value.

@Morikko

This comment has been minimized.

Show comment
Hide comment
@Morikko

Morikko Feb 7, 2018

Moved to #6754

{% assign guides = site.pages
    | where_exp: "item" , "item.title  contains 'Data' or item.title  contains 'Groups'"
 %}

I got : Error: Liquid syntax error (.../guide.html line 12): Expected end_of_string but found id included

Are the operators or and and not supported in where_exp ?

Note: actually and can be done by chaining many where_exp but not or.

Morikko commented Feb 7, 2018

Moved to #6754

{% assign guides = site.pages
    | where_exp: "item" , "item.title  contains 'Data' or item.title  contains 'Groups'"
 %}

I got : Error: Liquid syntax error (.../guide.html line 12): Expected end_of_string but found id included

Are the operators or and and not supported in where_exp ?

Note: actually and can be done by chaining many where_exp but not or.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Feb 7, 2018

Member

@Morikko This PR is pretty old. You may want to open a fresh issue.

Member

pathawks commented Feb 7, 2018

@Morikko This PR is pretty old. You may want to open a fresh issue.

@jekyll jekyll locked as resolved and limited conversation to collaborators Feb 7, 2018

@IgnoredAmbience IgnoredAmbience deleted the IgnoredAmbience:where-exp branch Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.