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 array support to `where` filter #4555

Merged
merged 4 commits into from Mar 9, 2016

Conversation

Projects
None yet
4 participants
@timwis
Contributor

timwis commented Feb 21, 2016

In addition to where supporting basic string matches (ie. category == "foo") this allows the where filter to work on array values, ie. category contains "foo". So whether the page has

---
category: Foo

---

or

---
category:
  - Foo

---

The where filter will still work ({{ site.pages | where:"category", "foo" }})

Note that this should not have an effect on any existing (string-match) use of where (non-breaking change)

Resolves #4385 (feature request)

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Feb 24, 2016

Member

This changes the behaviour of this filter in possibly nefarious ways because it'd be grabbing things that previously it didn't. @benbalter, what say you? If we can't come up with an example where it'd be a problem, then I'm 👍.

Member

parkr commented Feb 24, 2016

This changes the behaviour of this filter in possibly nefarious ways because it'd be grabbing things that previously it didn't. @benbalter, what say you? If we can't come up with an example where it'd be a problem, then I'm 👍.

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Feb 24, 2016

Contributor

I can't think of an example where this would break existing behavior (also 👋 @timwis).

Contributor

benbalter commented Feb 24, 2016

I can't think of an example where this would break existing behavior (also 👋 @timwis).

@timwis

This comment has been minimized.

Show comment
Hide comment
@timwis

timwis Feb 24, 2016

Contributor

Hey @benbalter! Thanks for the review guys.

Contributor

timwis commented Feb 24, 2016

Hey @benbalter! Thanks for the review guys.

@jekyllbot

This comment has been minimized.

Show comment
Hide comment
@jekyllbot

jekyllbot Feb 24, 2016

Contributor

@timwis It appears our Travis-CI check has failed. Did you add a test for your change and ensure preexisting tests pass as before? If not, please do so. Thank you! 🙏

Contributor

jekyllbot commented Feb 24, 2016

@timwis It appears our Travis-CI check has failed. Did you add a test for your change and ensure preexisting tests pass as before? If not, please do so. Thank you! 🙏

@timwis

This comment has been minimized.

Show comment
Hide comment
@timwis

timwis Feb 25, 2016

Contributor

Okay, figured out why the tests were failing and rewrote the PR to account for all property types. I also added 2 tests. I'm new to ruby so please let me know if I've made any convention mistakes or anything.

Contributor

timwis commented Feb 25, 2016

Okay, figured out why the tests were failing and rewrote the PR to account for all property types. I also added 2 tests. I'm new to ruby so please let me know if I've made any convention mistakes or anything.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 1, 2016

Member

What happens if you have category: bear and you search for where: "category", "ear"? From my reading of the code, you'd get back that post with category: bear even though you weren't really interested in that.

Member

parkr commented Mar 1, 2016

What happens if you have category: bear and you search for where: "category", "ear"? From my reading of the code, you'd get back that post with category: bear even though you weren't really interested in that.

@timwis

This comment has been minimized.

Show comment
Hide comment
@timwis

timwis Mar 3, 2016

Contributor

Hey @parkr, perhaps I'm misunderstanding, but I don't think that's the case. I've added a test for it to demonstrate. Let me know if I'm off.

Contributor

timwis commented Mar 3, 2016

Hey @parkr, perhaps I'm misunderstanding, but I don't think that's the case. I've added a test for it to demonstrate. Let me know if I'm off.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 9, 2016

Member

This LGTM. Thanks!

Member

parkr commented Mar 9, 2016

This LGTM. Thanks!

@parkr parkr added this to the 3.2 milestone Mar 9, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 9, 2016

Member

@jekyllbot: merge +minor

Member

parkr commented Mar 9, 2016

@jekyllbot: merge +minor

jekyllbot added a commit that referenced this pull request Mar 9, 2016

@jekyllbot jekyllbot merged commit 9e0ed00 into jekyll:master Mar 9, 2016

1 check passed

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

jekyllbot added a commit that referenced this pull request Mar 9, 2016

@timwis timwis deleted the timwis:patch-1 branch Apr 16, 2016

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