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

Adds group_by_exp filter #5513

Merged
merged 5 commits into from Dec 10, 2016

Conversation

Projects
None yet
6 participants
@thiagoarrais
Contributor

thiagoarrais commented Oct 25, 2016

Tasklist:

  • {% data | group_by_exp: "item", "item" %}
  • {% data | group_by_exp: "item", "item.version" %}
  • {% data | group_by_exp: "item", "item.version | split: '.' | first" %}
  • update docs

Fixes #5415.

@thiagoarrais thiagoarrais changed the title from Adds group_by_exp filter to WIP: Adds group_by_exp filter Oct 25, 2016

@pathawks

literally nothing

😆

@pathawks pathawks added this to the flexible milestone Oct 25, 2016

@pathawks pathawks added the feature label Oct 25, 2016

@DirtyF DirtyF added the needs-work label Oct 25, 2016

@thiagoarrais thiagoarrais referenced this pull request Oct 26, 2016

Closed

Missing data point? #62

@jekyllbot jekyllbot removed the needs-work label Oct 26, 2016

@thiagoarrais

This comment has been minimized.

Show comment
Hide comment
@thiagoarrais

thiagoarrais Oct 26, 2016

Contributor

I'm thinking I need a parse_expression method here. But I'm not sure. Maybe modifying the parse_condition method will do?

Contributor

thiagoarrais commented Oct 26, 2016

I'm thinking I need a parse_expression method here. But I'm not sure. Maybe modifying the parse_condition method will do?

@thiagoarrais

This comment has been minimized.

Show comment
Hide comment
@thiagoarrais
Contributor

thiagoarrais commented Oct 26, 2016

i have no idea what i'm doing dog

@thiagoarrais

This comment has been minimized.

Show comment
Hide comment
@thiagoarrais

thiagoarrais Oct 31, 2016

Contributor

Current version actually works and groups stuff. I need to learn one or thing or two about Liquid parsing to implement @caspervonb's example fully, though. I'm working on that.

Rubocop is also complaning about the size of lib/jekyll/filters.rb. We'll need to get that fixed before merging the PR, right? Any suggestions?

Contributor

thiagoarrais commented Oct 31, 2016

Current version actually works and groups stuff. I need to learn one or thing or two about Liquid parsing to implement @caspervonb's example fully, though. I'm working on that.

Rubocop is also complaning about the size of lib/jekyll/filters.rb. We'll need to get that fixed before merging the PR, right? Any suggestions?

@thiagoarrais

This comment has been minimized.

Show comment
Hide comment
@thiagoarrais

thiagoarrais Nov 1, 2016

Contributor

@caspervonb: can you take a look if this is what you had in mind?

Contributor

thiagoarrais commented Nov 1, 2016

@caspervonb: can you take a look if this is what you had in mind?

@caspervonb

This comment has been minimized.

Show comment
Hide comment
@caspervonb

caspervonb Nov 1, 2016

itens... OCD 😭

The result looks correct but we should probaly yield the same object type as group_by? A group has a name (the value it was grouped by), an array of items and a size (group.items.size == group.size)

Relevant group_by tests
https://github.com/thiagoarrais/jekyll/blob/5415-group_by_exp-filter/test/test_filters.rb#L595

should "allow filters in expression" do
  items = [
    { "version"=>"1.0", "result"=>"slow" },
    { "version"=>"1.1.5", "result"=>"medium" },
    { "version"=>"2.7.3", "result"=>"fast" }
  ]

  result = @filter.group_by_exp(items, "item", "item.version | split: '.' | first")
  expected = [
    {
      "name" => "1",
      "items" => [
        { "version"=>"1.0", "result"=>"slow" },  
        { "version"=>"1.1.5", "result"=>"medium" },
      ]
    },
    {
      "name" => "2",
      "items" => { "version"=>"2.7.3", "result"=>"fast" }
    },
  ]

  assert_equal expected, result
end

But yeah its grouping by the expression, filters and all so 👍

caspervonb commented Nov 1, 2016

itens... OCD 😭

The result looks correct but we should probaly yield the same object type as group_by? A group has a name (the value it was grouped by), an array of items and a size (group.items.size == group.size)

Relevant group_by tests
https://github.com/thiagoarrais/jekyll/blob/5415-group_by_exp-filter/test/test_filters.rb#L595

should "allow filters in expression" do
  items = [
    { "version"=>"1.0", "result"=>"slow" },
    { "version"=>"1.1.5", "result"=>"medium" },
    { "version"=>"2.7.3", "result"=>"fast" }
  ]

  result = @filter.group_by_exp(items, "item", "item.version | split: '.' | first")
  expected = [
    {
      "name" => "1",
      "items" => [
        { "version"=>"1.0", "result"=>"slow" },  
        { "version"=>"1.1.5", "result"=>"medium" },
      ]
    },
    {
      "name" => "2",
      "items" => { "version"=>"2.7.3", "result"=>"fast" }
    },
  ]

  assert_equal expected, result
end

But yeah its grouping by the expression, filters and all so 👍

@thiagoarrais

This comment has been minimized.

Show comment
Hide comment
@thiagoarrais

thiagoarrais Nov 1, 2016

Contributor

itens... OCD 😭

😆

Contributor

thiagoarrais commented Nov 1, 2016

itens... OCD 😭

😆

@thiagoarrais

This comment has been minimized.

Show comment
Hide comment
@thiagoarrais

thiagoarrais Nov 1, 2016

Contributor

Awesome, @caspervonb! Will take a look at that next. See 679a0c1#diff-77a9bfa5e00815266a775b7f497e3595

Contributor

thiagoarrais commented Nov 1, 2016

Awesome, @caspervonb! Will take a look at that next. See 679a0c1#diff-77a9bfa5e00815266a775b7f497e3595

@caspervonb

This comment has been minimized.

Show comment
Hide comment
@caspervonb

caspervonb Nov 2, 2016

These tests should cover everything I think, tho may want to add some more coverage for more complex expressions

context "group_by_exp filter" do
  should "successfully group array of Jekyll::Page's" do
    @filter.site.process
    grouping = @filter.group_by_exp(@filter.site.pages, "page", "page.layout | upcase")
    grouping.each do |g|
      assert(
        ["DEFAULT", "NIL", ""].include?(g["name"]),
        "#{g["name"]} isn't a valid grouping."
      )
      case g["name"]
      when "DEFAULT"
        assert(
          g["items"].is_a?(Array),
          "The list of grouped items for 'default' is not an Array."
        )
        assert_equal 5, g["items"].size
      when "nil"
        assert(
          g["items"].is_a?(Array),
          "The list of grouped items for 'nil' is not an Array."
        )
        assert_equal 2, g["items"].size
      when ""
        assert(
          g["items"].is_a?(Array),
          "The list of grouped items for '' is not an Array."
        )
        assert_equal 15, g["items"].size
      end
    end

  should "include the size of each grouping" do
    grouping = @filter.group_by_exp(@filter.site.pages, "page", "page.layout")
    grouping.each do |g|
      p g
      assert_equal(
        g["items"].size,
        g["size"],
        "The size property for '#{g["name"]}' doesn't match the size of the Array."
      )
    end
  end

  should "be equivalent of group_by" do
    grouping = @filter.group_by_exp(@filter.site.pages, "page", "page.layout")
    expected = @filter.group_by(@filter.site.pages, "layout")

    assert_equal grouping, expected
  end
end

caspervonb commented Nov 2, 2016

These tests should cover everything I think, tho may want to add some more coverage for more complex expressions

context "group_by_exp filter" do
  should "successfully group array of Jekyll::Page's" do
    @filter.site.process
    grouping = @filter.group_by_exp(@filter.site.pages, "page", "page.layout | upcase")
    grouping.each do |g|
      assert(
        ["DEFAULT", "NIL", ""].include?(g["name"]),
        "#{g["name"]} isn't a valid grouping."
      )
      case g["name"]
      when "DEFAULT"
        assert(
          g["items"].is_a?(Array),
          "The list of grouped items for 'default' is not an Array."
        )
        assert_equal 5, g["items"].size
      when "nil"
        assert(
          g["items"].is_a?(Array),
          "The list of grouped items for 'nil' is not an Array."
        )
        assert_equal 2, g["items"].size
      when ""
        assert(
          g["items"].is_a?(Array),
          "The list of grouped items for '' is not an Array."
        )
        assert_equal 15, g["items"].size
      end
    end

  should "include the size of each grouping" do
    grouping = @filter.group_by_exp(@filter.site.pages, "page", "page.layout")
    grouping.each do |g|
      p g
      assert_equal(
        g["items"].size,
        g["size"],
        "The size property for '#{g["name"]}' doesn't match the size of the Array."
      )
    end
  end

  should "be equivalent of group_by" do
    grouping = @filter.group_by_exp(@filter.site.pages, "page", "page.layout")
    expected = @filter.group_by(@filter.site.pages, "layout")

    assert_equal grouping, expected
  end
end
Show outdated Hide outdated lib/jekyll/filters.rb
# "items" => [...] } # all the items where `property` == "larry"
def group_by(input, property)
if groupable?(input)
groups = input.group_by { |item| item_property(item, property).to_s }

This comment has been minimized.

@thiagoarrais

thiagoarrais Nov 4, 2016

Contributor

I did this mostly because Rubocop was complaining about module length. But I'm torn about this refactoring mostly because of this call to item_property. This line got moved to the new module, but keeps calling code on the original one. In other words, the new module is bound to be included by the old one.

Should we keep this as per this commit? Should we extract this common code to a third module? Should we break the original Filters module in a completely different way?

@thiagoarrais

thiagoarrais Nov 4, 2016

Contributor

I did this mostly because Rubocop was complaining about module length. But I'm torn about this refactoring mostly because of this call to item_property. This line got moved to the new module, but keeps calling code on the original one. In other words, the new module is bound to be included by the old one.

Should we keep this as per this commit? Should we extract this common code to a third module? Should we break the original Filters module in a completely different way?

@thiagoarrais

This comment has been minimized.

Show comment
Hide comment
@thiagoarrais

thiagoarrais Nov 4, 2016

Contributor

I'm mostly happy about this PR now. But some feedback on the way I split the Filters module would be awesome. See 7f18ac8#r86635564.

Contributor

thiagoarrais commented Nov 4, 2016

I'm mostly happy about this PR now. But some feedback on the way I split the Filters module would be awesome. See 7f18ac8#r86635564.

@thiagoarrais thiagoarrais changed the title from WIP: Adds group_by_exp filter to Adds group_by_exp filter Nov 4, 2016

@thiagoarrais

This comment has been minimized.

Show comment
Hide comment
@thiagoarrais

thiagoarrais Nov 4, 2016

Contributor

This is done at 7f18ac8. Let me know if you need any help merging this.

Contributor

thiagoarrais commented Nov 4, 2016

This is done at 7f18ac8. Let me know if you need any help merging this.

@caspervonb

This comment has been minimized.

Show comment
Hide comment
@caspervonb

caspervonb Nov 4, 2016

I'm mostly happy about this PR now. But some feedback on the way I split the Filters module would be awesome. See 379f917#r86533966.

Think you merged that link/commitish into non existence ;)

caspervonb commented Nov 4, 2016

I'm mostly happy about this PR now. But some feedback on the way I split the Filters module would be awesome. See 379f917#r86533966.

Think you merged that link/commitish into non existence ;)

@thiagoarrais

This comment has been minimized.

Show comment
Hide comment
@thiagoarrais

thiagoarrais Nov 4, 2016

Contributor

Yeah. Will rebuild it.

Contributor

thiagoarrais commented Nov 4, 2016

Yeah. Will rebuild it.

@caspervonb

This comment has been minimized.

Show comment
Hide comment
@caspervonb

caspervonb Nov 4, 2016

Might as-well do RDoc comments too!

# Group an array of items by an expression
#
# input - the object array
# variable - the variable to assign each item to in the expression 
# expression -a Liquid comparison expression passed in as a string
#
# Returns the filtered array of objects

caspervonb commented Nov 4, 2016

Might as-well do RDoc comments too!

# Group an array of items by an expression
#
# input - the object array
# variable - the variable to assign each item to in the expression 
# expression -a Liquid comparison expression passed in as a string
#
# Returns the filtered array of objects
# "items" => [...] } # all the items where `property` == "larry"
def group_by(input, property)
if groupable?(input)
groups = input.group_by { |item| item_property(item, property).to_s }

This comment has been minimized.

@thiagoarrais

thiagoarrais Nov 4, 2016

Contributor

I did this to make Rubocop happy. But I'm torn about this refactoring and the main reason is this call to item_property. It binds the new module to being included to the old one.

Should we keep this as is? Should we split item_property to its own module? Should we split Filters in a totally different way?

@thiagoarrais

thiagoarrais Nov 4, 2016

Contributor

I did this to make Rubocop happy. But I'm torn about this refactoring and the main reason is this call to item_property. It binds the new module to being included to the old one.

Should we keep this as is? Should we split item_property to its own module? Should we split Filters in a totally different way?

@thiagoarrais

This comment has been minimized.

Show comment
Hide comment
@thiagoarrais

thiagoarrais Nov 4, 2016

Contributor

Yeah, sure!

Contributor

thiagoarrais commented Nov 4, 2016

Yeah, sure!

@thiagoarrais

This comment has been minimized.

Show comment
Hide comment
@thiagoarrais

thiagoarrais Nov 4, 2016

Contributor

@caspervonb: I fixed the link and edited the comment. Sorry.

Contributor

thiagoarrais commented Nov 4, 2016

@caspervonb: I fixed the link and edited the comment. Sorry.

@caspervonb

LGTM

@parkr

parkr approved these changes Nov 29, 2016

This LGTM once CI passes. 👌

Show outdated Hide outdated lib/jekyll/filters/grouping_filters.rb
end
private
def make_grouped_array(groups)

This comment has been minimized.

@parkr

parkr Nov 29, 2016

Member

I don't like the name of this method – something about Ruby conventions which I always loved is that the method could be considered like a variable, so you name things as though they aren't dynamic. In this case, you'd remove make_ and just have grouped_array(groups) or group_metadata_array(groups). Would either of those suit you?

@parkr

parkr Nov 29, 2016

Member

I don't like the name of this method – something about Ruby conventions which I always loved is that the method could be considered like a variable, so you name things as though they aren't dynamic. In this case, you'd remove make_ and just have grouped_array(groups) or group_metadata_array(groups). Would either of those suit you?

This comment has been minimized.

@thiagoarrais

thiagoarrais Nov 30, 2016

Contributor

something about Ruby conventions which I always loved is that the method could be considered like a variable

Totally agree! That is something I appreciate in Ruby too.

I guess I've been dayjobbing on Javaland for too long, though. ;-)

Would either of those suit you?

Sure. I chose grouped_array(groups). See 91f0b91

@thiagoarrais

thiagoarrais Nov 30, 2016

Contributor

something about Ruby conventions which I always loved is that the method could be considered like a variable

Totally agree! That is something I appreciate in Ruby too.

I guess I've been dayjobbing on Javaland for too long, though. ;-)

Would either of those suit you?

Sure. I chose grouped_array(groups). See 91f0b91

Show outdated Hide outdated test/test_filters.rb
should "include the size of each grouping" do
groups = @filter.group_by_exp(@filter.site.pages, "page", "page.layout")
groups.each do |g|
p g

This comment has been minimized.

@parkr

parkr Nov 29, 2016

Member

Whoops!

@parkr

parkr Nov 29, 2016

Member

Whoops!

This comment has been minimized.

@thiagoarrais
@thiagoarrais

thiagoarrais added some commits Nov 30, 2016

@thiagoarrais

This comment has been minimized.

Show comment
Hide comment
@thiagoarrais

thiagoarrais Dec 9, 2016

Contributor

@parkr I've finally got around to fixing this. CI is now passing. Let me know if you want this to be squashed/rebased.

Contributor

thiagoarrais commented Dec 9, 2016

@parkr I've finally got around to fixing this. CI is now passing. Let me know if you want this to be squashed/rebased.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 10, 2016

Member

Looks great to me, thank you!

@jekyllbot: merge +minor

Member

parkr commented Dec 10, 2016

Looks great to me, thank you!

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 8ed3240 into jekyll:master Dec 10, 2016

2 checks passed

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

jekyllbot added a commit that referenced this pull request Dec 10, 2016

@thiagoarrais thiagoarrais deleted the thiagoarrais:5415-group_by_exp-filter branch Dec 10, 2016

@caspervonb

This comment has been minimized.

Show comment
Hide comment
@caspervonb

caspervonb Dec 10, 2016

Great, thanks for the work @thiagoarrais 👍

caspervonb commented Dec 10, 2016

Great, thanks for the work @thiagoarrais 👍

@thiagoarrais

This comment has been minimized.

Show comment
Hide comment
@thiagoarrais

thiagoarrais Dec 10, 2016

Contributor

You're welcome, @caspervonb. Let me know if you choose to use this in jsperf.

Contributor

thiagoarrais commented Dec 10, 2016

You're welcome, @caspervonb. Let me know if you choose to use this in jsperf.

@dylanahsmith dylanahsmith referenced this pull request Dec 16, 2016

Merged

Upgrade to Liquid v4 #4362

parkr added a commit that referenced this pull request Dec 16, 2016

parkr added a commit that referenced this pull request Dec 16, 2016

Merge branch 'master' into liquid-4
* master: (40 commits)
  Update history to reflect merge of #5658 [ci skip]
  Fix a couple of typos in the docs
  Update history to reflect merge of #5657 [ci skip]
  Update variables.md
  Update history to reflect merge of #5653 [ci skip]
  Improve Permalinks documentation.
  Update history to reflect merge of #5652 [ci skip]
  Use `assert_nil` instead of `assert_equal nil`
  Update history to reflect merge of #5513 [ci skip]
  Ignore symlinked file in windows
  Update history to reflect merge of #5612 [ci skip]
  Update history to reflect merge of #5643 [ci skip]
  Update Core team list in README
  Update history to reflect merge of #5641 [ci skip]
  use backticks for Gemfile for consistency since in the next sentence _config.yml file has backtick
  add a set of steps in site_configuration.feature
  update documentation for Windows
  narrow it down to only Windows
  revert and adjust site_configuration.feature
  add 'tzinfo-data' gem to generated Gemfile
  ...

@DirtyF DirtyF removed this from the flexible milestone Nov 10, 2017

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