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

rubocop: jekyll/lib/filters.rb #4993

Merged
merged 1 commit into from Jun 14, 2016

Conversation

Projects
None yet
5 participants
@ayastreb
Contributor

ayastreb commented Jun 7, 2016

#4885

Show outdated Hide outdated lib/jekyll/filters.rb
item_property(item, property).to_s
end.inject([]) do |memo, i|
end
grouped.inject([]) do |memo, i|

This comment has been minimized.

@envygeeks

envygeeks Jun 7, 2016

Contributor
input.group_by { |v| item_property(v, property).to_s }.
  each_with_object([]) do |item, ary|
    array << { 
      "name" => item.first, 
      "size" => item.last.size 
      "items" => item.last, 
    }
  end
@envygeeks

envygeeks Jun 7, 2016

Contributor
input.group_by { |v| item_property(v, property).to_s }.
  each_with_object([]) do |item, ary|
    array << { 
      "name" => item.first, 
      "size" => item.last.size 
      "items" => item.last, 
    }
  end
@@ -223,7 +224,9 @@ def group_by(input, property)
def where(input, property, value)
return input unless input.is_a?(Enumerable)
input = input.values if input.is_a?(Hash)
input.select { |object| Array(item_property(object, property)).map(&:to_s).include?(value.to_s) }
input.select do |object|
Array(item_property(object, property)).map(&:to_s).include?(value.to_s)

This comment has been minimized.

@envygeeks

envygeeks Jun 7, 2016

Contributor

[item_property(object, property)]

@envygeeks

envygeeks Jun 7, 2016

Contributor

[item_property(object, property)]

This comment has been minimized.

@ayastreb

ayastreb Jun 7, 2016

Contributor

Sorry, didn't get it 😕
You suggest to replace Array(item_property(object, property)).map(&:to_s).include?(value.to_s) line with [item_property(object, property)].map(&:to_s).include?(value.to_s) to make it shorter?

@ayastreb

ayastreb Jun 7, 2016

Contributor

Sorry, didn't get it 😕
You suggest to replace Array(item_property(object, property)).map(&:to_s).include?(value.to_s) line with [item_property(object, property)].map(&:to_s).include?(value.to_s) to make it shorter?

This comment has been minimized.

@envygeeks

envygeeks Jun 7, 2016

Contributor

Only Array() should be replaced with []

@envygeeks

envygeeks Jun 7, 2016

Contributor

Only Array() should be replaced with []

This comment has been minimized.

@ayastreb

ayastreb Jun 7, 2016

Contributor

I tried that, so the method looks like this:

def where(input, property, value)
  return input unless input.is_a?(Enumerable)
  input = input.values if input.is_a?(Hash)
  input.select do |object|
    [item_property(object, property)].map(&:to_s).include?(value.to_s)
  end
end

but then 2 tests fail:

Failure:
TestFilters#test_: filters where filter should filter array properties appropriately.  [test/test_filters.rb:472]
Minitest::Assertion: Expected: 2
  Actual: 0

Failure:
TestFilters#test_: filters where filter should filter array properties alongside string properties.  [test/test_filters.rb:481]
Minitest::Assertion: Expected: 2
  Actual: 1
@ayastreb

ayastreb Jun 7, 2016

Contributor

I tried that, so the method looks like this:

def where(input, property, value)
  return input unless input.is_a?(Enumerable)
  input = input.values if input.is_a?(Hash)
  input.select do |object|
    [item_property(object, property)].map(&:to_s).include?(value.to_s)
  end
end

but then 2 tests fail:

Failure:
TestFilters#test_: filters where filter should filter array properties appropriately.  [test/test_filters.rb:472]
Minitest::Assertion: Expected: 2
  Actual: 0

Failure:
TestFilters#test_: filters where filter should filter array properties alongside string properties.  [test/test_filters.rb:481]
Minitest::Assertion: Expected: 2
  Actual: 1

This comment has been minimized.

@envygeeks

envygeeks Jun 7, 2016

Contributor

Can you post a copy of the test?

@envygeeks

envygeeks Jun 7, 2016

Contributor

Can you post a copy of the test?

This comment has been minimized.

@ayastreb

ayastreb Jun 7, 2016

Contributor

Sure, it's this one:

should "filter array properties appropriately" do
  hash = {
    "a" => { "tags"=>%w(x y) },
    "b" => { "tags"=>["x"] },
    "c" => { "tags"=>%w(y z) }
  }
  assert_equal 2, @filter.where(hash, "tags", "x").length
end

and also this one:

should "filter array properties alongside string properties" do
  hash = {
    "a" => { "tags"=>%w(x y) },
    "b" => { "tags"=>"x" },
    "c" => { "tags"=>%w(y z) }
  }
  assert_equal 2, @filter.where(hash, "tags", "x").length
end
@ayastreb

ayastreb Jun 7, 2016

Contributor

Sure, it's this one:

should "filter array properties appropriately" do
  hash = {
    "a" => { "tags"=>%w(x y) },
    "b" => { "tags"=>["x"] },
    "c" => { "tags"=>%w(y z) }
  }
  assert_equal 2, @filter.where(hash, "tags", "x").length
end

and also this one:

should "filter array properties alongside string properties" do
  hash = {
    "a" => { "tags"=>%w(x y) },
    "b" => { "tags"=>"x" },
    "c" => { "tags"=>%w(y z) }
  }
  assert_equal 2, @filter.where(hash, "tags", "x").length
end

This comment has been minimized.

@envygeeks

envygeeks Jun 7, 2016

Contributor

Sorry for the late reply @ayastreb, I didn't get an email for some reason and just randomly checked to see if this had feedback. That said, I guess keep it as Array, I didn't think they were doing a cheap flatten, but it seems to me now that they are. I don't quite agree since [blah].flatten(1) is more explicit to a non-programmer but I digress.

Lets get some context on this from @parkr or anybody else on @jekyll/core who might have more info.

@envygeeks

envygeeks Jun 7, 2016

Contributor

Sorry for the late reply @ayastreb, I didn't get an email for some reason and just randomly checked to see if this had feedback. That said, I guess keep it as Array, I didn't think they were doing a cheap flatten, but it seems to me now that they are. I don't quite agree since [blah].flatten(1) is more explicit to a non-programmer but I digress.

Lets get some context on this from @parkr or anybody else on @jekyll/core who might have more info.

@@ -333,6 +324,22 @@ def inspect(input)
end
private
def sort_input(input, property, order)
input.sort do |apple, orange|

This comment has been minimized.

@envygeeks

envygeeks Jun 7, 2016

Contributor

This should simply do "a, b", I like the apple and orange though!

@envygeeks

envygeeks Jun 7, 2016

Contributor

This should simply do "a, b", I like the apple and orange though!

@@ -363,6 +372,7 @@ def item_property(item, property)
end
end
private

This comment has been minimized.

@envygeeks

envygeeks Jun 7, 2016

Contributor

AFAIW as liquid should be public, /cc @jekyll/core is this assumption right?

@envygeeks

envygeeks Jun 7, 2016

Contributor

AFAIW as liquid should be public, /cc @jekyll/core is this assumption right?

This comment has been minimized.

@pathawks

pathawks Jun 7, 2016

Member

These are all just helper methods that were already private 👍

@pathawks

pathawks Jun 7, 2016

Member

These are all just helper methods that were already private 👍

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 7, 2016

Contributor

Thanks! Just a few comments, some for you, some for @jekyll/core.

Contributor

envygeeks commented Jun 7, 2016

Thanks! Just a few comments, some for you, some for @jekyll/core.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 14, 2016

Member

Hey @ayastreb! Thank you for doing this. ❤️ This looks pretty good to me so far. One question: why is private specified so many times? Is there a way we could specify it once? Or was it done to be more explicit? We don't do that anywhere else in the code so we should be consistent one way or the other. Thanks!

Member

parkr commented Jun 14, 2016

Hey @ayastreb! Thank you for doing this. ❤️ This looks pretty good to me so far. One question: why is private specified so many times? Is there a way we could specify it once? Or was it done to be more explicit? We don't do that anywhere else in the code so we should be consistent one way or the other. Thanks!

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jun 14, 2016

Contributor

@parkr it was done at my request because it makes the code more explicit and audit-able by quick glance.

Contributor

envygeeks commented Jun 14, 2016

@parkr it was done at my request because it makes the code more explicit and audit-able by quick glance.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 14, 2016

Member

@envygeeks Ok, sounds great!

@jekyllbot: merge +dev

Member

parkr commented Jun 14, 2016

@envygeeks Ok, sounds great!

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 9f1f702 into jekyll:master Jun 14, 2016

1 check passed

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

jekyllbot added a commit that referenced this pull request Jun 14, 2016

@parkr parkr removed the pending-feedback label Jun 14, 2016

@parkr parkr referenced this pull request Jun 15, 2016

Closed

Make jekyll/jekyll code compliant with rubocop rules #4885

114 of 115 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment