Allow filters to sort & select based on subvalues #5622
Conversation
|
@gnuletik can also add to the |
|
Hi ! Any news on this ? |
|
/cc: @jekyll/build |
lib/jekyll/filters.rb
Outdated
| property.to_s.split(".").each do |attr| | ||
| res = res[attr] | ||
| end | ||
| res | ||
| elsif item.respond_to?(:data) | ||
| item.data[property.to_s] |
There was a problem hiding this comment.
Wouldn't that also need to happen here?
There was a problem hiding this comment.
It's what I was wondering in my first topic, but isn't it used for data files ? If yes subvalues may already be supported
There was a problem hiding this comment.
You could test it out (via CI and manually) to confirm..
There was a problem hiding this comment.
@ashmaroli You are right, it wasn't working for data files.
Here are my tests:
_data/test/test.json
{
"array": [
{"header": { "year": 2001 }},
{"header": { "year": 2004 }},
{"header": { "year": 2006 }},
{"header": { "year": 1972 }},
{"header": { "year": 2010 }},
{"header": { "year": 1990 }},
{"header": { "year": 12 }},
{"header": { "year": 0 }},
{"header": { "year": 1996 }}
]
}
index.html
---
---
{% assign test = site.data.test.array | sort: 'header.year' %}
{{ test }}
{% for i in test %}
{{ i.header.year }}
{% endfor %}
Commands :
$ jekyll -v
jekyll 3.4.3
$ jekyll build && cat _site/index.html
Configuration file: none
Source: /tmp/test-site
Destination: /tmp/test-site/_site
Incremental build: disabled. Enable with --incremental
Generating...
done in 0.01 seconds.
Auto-regeneration: disabled. Use --watch to enable.
{"header"=>{"year"=>2001}}{"header"=>{"year"=>2004}}{"header"=>{"year"=>2006}}{"header"=>{"year"=>1972}}{"header"=>{"year"=>2010}}{"header"=>{"year"=>1990}}{"header"=>{"year"=>12}}{"header"=>{"year"=>0}}{"header"=>{"year"=>1996}}
2001
2004
2006
1972
2010
1990
12
0
1996
With the last commit :
$ ../jekyll/exe/jekyll build && cat _site/index.html
C
10000
onfiguration file: none
Source: /tmp/test-site
Destination: /tmp/test-site/_site
Incremental build: disabled. Enable with --incremental
Generating...
done in 0.009 seconds.
Auto-regeneration: disabled. Use --watch to enable.
{"header"=>{"year"=>0}}{"header"=>{"year"=>12}}{"header"=>{"year"=>1972}}{"header"=>{"year"=>1990}}{"header"=>{"year"=>1996}}{"header"=>{"year"=>2001}}{"header"=>{"year"=>2004}}{"header"=>{"year"=>2006}}{"header"=>{"year"=>2010}}
0
12
1972
1990
1996
2001
2004
2006
2010
There was a problem hiding this comment.
That's great! Could you add a similar test to our test-suite as well? Thanks 🙂
There was a problem hiding this comment.
Yeah I would like to but in the test/test_filters.rb file I can't figure out how can I give the input as an object responding to data.
I looked for other filters test and there is no filters testing for data inputs.
There was a problem hiding this comment.
@ashmaroli Also, I'm trying to find in which use case the else condition is being called (where the input does not have .data or .to_liquid) but I can't find.
May it be possible to identify tests going though thanks to code climate ? I didn't find how I can achieve that.
Thanks !
lib/jekyll/filters.rb
Outdated
| property.to_s.split(".").each do |attr| | ||
| res = res[attr] | ||
| end | ||
| res |
There was a problem hiding this comment.
This code can be cleaned up:
property.to_s.split(".").reduce(item.to_liquid) { |res, prop| res[prop] }There was a problem hiding this comment.
additionally, what is res? Is there a better name we could give it? I'm also not a giant fan of abbreviations, and would prefer to see property used in place of prop instead.
There was a problem hiding this comment.
I'm would use subvalue and attribute instead of res and prop but I can't because the line can't exceed 90 characters.
I also can't make it multine like this because it's not allowed in the rules.
property.to_s.split(".").reduce(item.to_liquid) { |val, attr|
val[attr]
}
There was a problem hiding this comment.
split_properties = property.to_s.split(".")
split_properties.reduce(item.to_liquid) do |subvalue, attribute|
subvalue[attribute]
endshould be good enough for rubocop
There was a problem hiding this comment.
Yes it is :)
I even removed split_properties = property.to_s.split(".") as rubocop allows 90 characters.
Thanks !
|
@gnuletik I ran a couple of tests locally using your sample json file and page, above against both manually creating a test site and the following: should "return sorted by subproperty data array" do
site = fixture_site
site.data.merge!({
"array" => [
{ "a" => { "b" => 2 } },
{ "a" => { "b" => 1 } },
{ "a" => { "b" => 3 } },
]
})
assert_equal(
[
{ "a" => { "b" => 1 } },
{ "a" => { "b" => 2 } },
{ "a" => { "b" => 3 } },
], @filter.sort(site.data["array"], "a.b")
)
end
endMy inference is that you don't have to patch @pathawks @parkr pinging you guys if could confirm my finding when possible |
|
@ashmaroli I did the tests too and your're right : patching |
|
So, is this version okay ? |
|
up |
|
@jekyllbot: merge +minor |
Hi,
I was trying to sort a collections using the
sortfilter like this{% assign collec = site.my_collection | sort 'header.year' %}but the subvalue wasn't working, so I made a quick fix.Maybe I should do the same thing for
item.data[property.to_s] and item[property.to_s]making a new method.What do you think about it ?
Thanks !