Skip to content
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 group_by liquid filter #2572

Merged
merged 2 commits into from
Aug 1, 2019
Merged

Conversation

elsurudo
Copy link
Contributor

Something which should really be a part of liquid core, but unfortunately isn't.

I had a need for this when using the LiquidOutputAgent to organize events by day.

@dsander
Copy link
Collaborator

dsander commented Jul 31, 2019

Nice, this looks very handy!

@dsander dsander requested a review from knu July 31, 2019 08:11
app/concerns/liquid_interpolatable.rb Outdated Show resolved Hide resolved
if input.respond_to?(:group_by)
[].tap do |grouped|
input.group_by { |item| item[property] }.each do |value, items|
grouped << { 'name' => value, 'items' => items }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not absolutely sure if name/items are the best choices here. key/values, maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that jekyll which has the same filter (although they have shared code between filters, thus I simplified it here and provided a simpler implementation).

Thus, I decided to use the same key names, since that is quite a popular project and thus provides some sort of "standard". I agree that it's a bit strange, but it makes sense when you think of it as "name of the group" and "items in the group". Liquid is meant for somewhat non-technical people and designers, after all.

But in the end, I'm not tied to anything. It's a judgement-call.

Copy link
Member

@knu knu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good reference. I second the choice then!

@dsander
Copy link
Collaborator

dsander commented Aug 1, 2019

Thank you! The CI failure was unrelated.

@dsander dsander merged commit 44cab36 into huginn:master Aug 1, 2019
@dsander
Copy link
Collaborator

dsander commented Aug 1, 2019

Almost forgot about it, can you add the filter to the wiki page?

@elsurudo
Copy link
Contributor Author

elsurudo commented Aug 2, 2019

All done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants