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

where_exp filter should filter posts #4860

Merged
merged 2 commits into from Jul 25, 2016

Conversation

Projects
None yet
5 participants
@pathawks
Member

pathawks commented Apr 30, 2016

{{ site.posts | where_exp: "posts", "posts.title == 'My Title'" }}

This just returns site.posts, it does not filter posts by title.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 2, 2016

Member

@pathawks If someone filed an issue for that here, we'd likely ask them to change it to {{ site.posts | where: "title", "My Title" }}. It's definitely a bug but also good to know that there's another way for this particular operation.

Is it all == operations? Could you assert_equal EXPECTED_SITE, results.size instead of comparing against site.posts? Is the result supposed to be empty?

Member

parkr commented May 2, 2016

@pathawks If someone filed an issue for that here, we'd likely ask them to change it to {{ site.posts | where: "title", "My Title" }}. It's definitely a bug but also good to know that there's another way for this particular operation.

Is it all == operations? Could you assert_equal EXPECTED_SITE, results.size instead of comparing against site.posts? Is the result supposed to be empty?

@parkr parkr added this to the 3.2 milestone May 2, 2016

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks May 2, 2016

Member

Ultimately, my use case is to get a list of all posts from the past 12 months. When the date stuff wasn’t working, I decided to see if I could get the where_exp filter to work at all, and I could not.

I know there is at least one post with the title Foo Bar. I will take another look tonight to see if I can make the test a bit tighter.

Member

pathawks commented May 2, 2016

Ultimately, my use case is to get a list of all posts from the past 12 months. When the date stuff wasn’t working, I decided to see if I could get the where_exp filter to work at all, and I could not.

I know there is at least one post with the title Foo Bar. I will take another look tonight to see if I can make the test a bit tighter.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 2, 2016

Member

Ok, thanks @pathawks 😄

You could assert that (1) the result is the right size (i.e. 1) and (2) that the result contains the post you're looking for (find the document yourself using site.posts.find {|p| p.title == "Foo Bar"} and compare)

Member

parkr commented May 2, 2016

Ok, thanks @pathawks 😄

You could assert that (1) the result is the right size (i.e. 1) and (2) that the result contains the post you're looking for (find the document yourself using site.posts.find {|p| p.title == "Foo Bar"} and compare)

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks May 24, 2016

Member

Changes have been made 👍

Test still fails, which is the point. The filter is not filtering, but just returning all posts.

Member

pathawks commented May 24, 2016

Changes have been made 👍

Test still fails, which is the point. The filter is not filtering, but just returning all posts.

@jeromecoupe

This comment has been minimized.

Show comment
Hide comment
@jeromecoupe

jeromecoupe Jun 2, 2016

Same here with Jekyll 3.1.6

  • This works fine: {% assign potsByAuthor = site.posts | where: 'author', page.teamId %}
  • Whereas this just returns all the posts {% assign potsByAuthor = site.posts | where_exp: 'item', 'item.author == page.teamId' %} The size of the potsByAuthor is always the total number of posts
  • Same if I try with a string: {% assign potsByAuthor = site.posts | where_exp: 'item', 'item.author == "jeromecoupe"' %}

jeromecoupe commented Jun 2, 2016

Same here with Jekyll 3.1.6

  • This works fine: {% assign potsByAuthor = site.posts | where: 'author', page.teamId %}
  • Whereas this just returns all the posts {% assign potsByAuthor = site.posts | where_exp: 'item', 'item.author == page.teamId' %} The size of the potsByAuthor is always the total number of posts
  • Same if I try with a string: {% assign potsByAuthor = site.posts | where_exp: 'item', 'item.author == "jeromecoupe"' %}
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 11, 2016

Member

Hm, tests aren't being triggered. Would you mind rebasing on jekyll:master to try to re-trigger the build (and resolve conflicts)?

Member

parkr commented Jul 11, 2016

Hm, tests aren't being triggered. Would you mind rebasing on jekyll:master to try to re-trigger the build (and resolve conflicts)?

@parkr parkr added pending-rebase and removed needs-work labels Jul 11, 2016

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jul 11, 2016

Member

Not sure how the failing test could be related (and only with Ruby 2.3). Would you mind restarting that build to see if it was just a fluke?

Member

pathawks commented Jul 11, 2016

Not sure how the failing test could be related (and only with Ruby 2.3). Would you mind restarting that build to see if it was just a fluke?

@pathawks pathawks changed the title from Failing test: where_exp filter should filter posts to where_exp filter should filter posts Jul 11, 2016

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jul 11, 2016

Member

Thanks! Looks like all tests pass now, and no conflicts with master

Member

pathawks commented Jul 11, 2016

Thanks! Looks like all tests pass now, and no conflicts with master

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 12, 2016

Member

@pathawks on mobile so can't double check, but do we actually test filtering site.posts? I bet drops behave strangely 😛

Member

parkr commented Jul 12, 2016

@pathawks on mobile so can't double check, but do we actually test filtering site.posts? I bet drops behave strangely 😛

Show outdated Hide outdated test/test_filters.rb
Show outdated Hide outdated test/test_filters.rb
@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks
Member

pathawks commented Jul 12, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 12, 2016

Member

Looks like we need to depend on activesupport v4.x for Ruby 2.1 and below?

Member

parkr commented Jul 12, 2016

Looks like we need to depend on activesupport v4.x for Ruby 2.1 and below?

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Jul 12, 2016

Member

@parkr Yes, would you be interested in a PR for this?

Member

Crunch09 commented Jul 12, 2016

@parkr Yes, would you be interested in a PR for this?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr
Member

parkr commented Jul 12, 2016

@Crunch09, Yes!

@parkr parkr added the fix label Jul 13, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 13, 2016

Member

#5100 is merged, would you mind rebasing?

Member

parkr commented Jul 13, 2016

#5100 is merged, would you mind rebasing?

pathawks and others added some commits Jul 14, 2016

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jul 14, 2016

Member
  • Rebased
Member

pathawks commented Jul 14, 2016

  • Rebased
posts = site.site_payload["site"]["posts"]
results = @filter.where_exp(posts, "obj", "obj.title == 'Foo Bar'")
assert_equal 1, results.length
assert_equal site.posts.find { |p| p.title == "Foo Bar" }, results.first

This comment has been minimized.

@parkr

parkr Jul 15, 2016

Member

Here's a curiosity: why isn't this comparing a Document with a DocumentDrop? Can you ensure results.first is, in fact, a DocumentDrop?

@parkr

parkr Jul 15, 2016

Member

Here's a curiosity: why isn't this comparing a Document with a DocumentDrop? Can you ensure results.first is, in fact, a DocumentDrop?

This comment has been minimized.

@pathawks

pathawks Jul 15, 2016

Member

results.first is a Document, not a DocumentDrop.
So... what should I do about that?

@pathawks

pathawks Jul 15, 2016

Member

results.first is a Document, not a DocumentDrop.
So... what should I do about that?

This comment has been minimized.

@parkr

parkr Jul 25, 2016

Member

Maybe the CollectionDrop should be recursively calling to_liquid on the documents it's accessing. When and how Liquid calls this method has always been a mystery.

@parkr

parkr Jul 25, 2016

Member

Maybe the CollectionDrop should be recursively calling to_liquid on the documents it's accessing. When and how Liquid calls this method has always been a mystery.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 15, 2016

Member

@pathawks ❤️

Member

parkr commented Jul 15, 2016

@pathawks ❤️

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 25, 2016

Member

@jekyllbot: merge +bug

Member

parkr commented Jul 25, 2016

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 0f3ce73 into jekyll:master Jul 25, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
jekyll/lgtm No approval is required.

@jekyllbot jekyllbot added bug fix labels Jul 25, 2016

jekyllbot added a commit that referenced this pull request Jul 25, 2016

@pathawks pathawks deleted the pathawks:pr/where_exp branch Jul 25, 2016

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jul 25, 2016

Member

I will not @mention in commit messages.
I will not @mention in commit messages.
I will not @mention in commit messages.

Member

pathawks commented Jul 25, 2016

I will not @mention in commit messages.
I will not @mention in commit messages.
I will not @mention in commit messages.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 25, 2016

Member

@pathawks ❤️

Member

parkr commented Jul 25, 2016

@pathawks ❤️

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