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

Fallback filter #1822

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@doktorbro
Member

doktorbro commented Dec 13, 2013

The fallback filter replaces nil input with a default value. If both are hashes they will be merged.

Usage in #1814:

{{ page.modified | fallback: page.date | date_to_xmlschema }}

Another example for #969 (comment) from @benbalter:

{{ page.author | fallback: "John Doe" }}

I even think the fallback filter gives the user a more straight forward tool than #1527. I don’t like the magic behind the default front-matter.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 13, 2013

Coverage Status

Coverage decreased (-0.08%) when pulling 2fc9369 on penibelst:add-merge-filter into b7bdcb1 on jekyll:master.

coveralls commented Dec 13, 2013

Coverage Status

Coverage decreased (-0.08%) when pulling 2fc9369 on penibelst:add-merge-filter into b7bdcb1 on jekyll:master.

@maul-esel

This comment has been minimized.

Show comment
Hide comment
@maul-esel

maul-esel Dec 13, 2013

Contributor

Looks nice. The code could probably be condensed a little like

if (input.kind_of?(Hash) && default.kind_of?(Hash))
  default.merge(input)
else
  input || default
end

but I'm not sure what coding style is preferred by the maintainers here.

I must note though that this can't substitute frontmatter defaults, as those also allow default layouts etc. (i.e. not just custom data, but also jekyll-interpreted settings). Also, the entire meaning of setting a default is not having to repeat it again and again, which is not tackled by this filter.

Contributor

maul-esel commented Dec 13, 2013

Looks nice. The code could probably be condensed a little like

if (input.kind_of?(Hash) && default.kind_of?(Hash))
  default.merge(input)
else
  input || default
end

but I'm not sure what coding style is preferred by the maintainers here.

I must note though that this can't substitute frontmatter defaults, as those also allow default layouts etc. (i.e. not just custom data, but also jekyll-interpreted settings). Also, the entire meaning of setting a default is not having to repeat it again and again, which is not tackled by this filter.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 13, 2013

Coverage Status

Coverage increased (+0.1%) when pulling 8ec273b on penibelst:add-merge-filter into b7bdcb1 on jekyll:master.

coveralls commented Dec 13, 2013

Coverage Status

Coverage increased (+0.1%) when pulling 8ec273b on penibelst:add-merge-filter into b7bdcb1 on jekyll:master.

@doktorbro

This comment has been minimized.

Show comment
Hide comment
@doktorbro

doktorbro Dec 13, 2013

Member

The code could probably be condensed a little

The filter should not override a false with the default value. This would happen in your code at line 4. I’ve added a test for that case.

BTW, maybe the filter label fallback is not the best choice. The word default would describe the behavior too. Feel free to propose a better label.

Member

doktorbro commented Dec 13, 2013

The code could probably be condensed a little

The filter should not override a false with the default value. This would happen in your code at line 4. I’ve added a test for that case.

BTW, maybe the filter label fallback is not the best choice. The word default would describe the behavior too. Feel free to propose a better label.

@maul-esel

This comment has been minimized.

Show comment
Hide comment
@maul-esel

maul-esel Dec 13, 2013

Contributor

The filter should not override a false with the default value. This would happen in your code at line 4. I’ve added a test for that case.

Right, I just saw your new commit.

I actually think "fallback" is pretty descriptive.

Contributor

maul-esel commented Dec 13, 2013

The filter should not override a false with the default value. This would happen in your code at line 4. I’ve added a test for that case.

Right, I just saw your new commit.

I actually think "fallback" is pretty descriptive.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 13, 2013

Member

This is a really great idea! Unfortunately, it looks like Liquid beat you to it: Shopify/liquid#267

Rel: #1666

Is the Liquid default filter what you're looking for?

Member

parkr commented Dec 13, 2013

This is a really great idea! Unfortunately, it looks like Liquid beat you to it: Shopify/liquid#267

Rel: #1666

Is the Liquid default filter what you're looking for?

@doktorbro

This comment has been minimized.

Show comment
Hide comment
@doktorbro

doktorbro Dec 14, 2013

Member

@parkr Yes, this looks exactly like my first draft from yesterday. But after realizing that it’s impossible to set input to false:

{{ false | fallback: "I win" }}
# => "I win"

I’ve changed my conditions to override only nil values.

But I could live with Liquid’s solution. Is the default filter already available in Jekyll?

Member

doktorbro commented Dec 14, 2013

@parkr Yes, this looks exactly like my first draft from yesterday. But after realizing that it’s impossible to set input to false:

{{ false | fallback: "I win" }}
# => "I win"

I’ve changed my conditions to override only nil values.

But I could live with Liquid’s solution. Is the default filter already available in Jekyll?

@doktorbro doktorbro closed this Dec 14, 2013

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 14, 2013

Member

No, it has not been released yet. /cc @fw42

If you'd like to duplicate that code for now if you would like, but we're hoping to release Jekyll v2.0.0 with Liquid v3.x support.

Member

parkr commented Dec 14, 2013

No, it has not been released yet. /cc @fw42

If you'd like to duplicate that code for now if you would like, but we're hoping to release Jekyll v2.0.0 with Liquid v3.x support.

@doktorbro

This comment has been minimized.

Show comment
Hide comment
@doktorbro

doktorbro Dec 14, 2013

Member

@parkr Thanks. I can wait until v2.0.0.

Member

doktorbro commented Dec 14, 2013

@parkr Thanks. I can wait until v2.0.0.

@doktorbro doktorbro deleted the doktorbro:add-merge-filter branch Dec 19, 2013

@benbalter benbalter referenced this pull request Jun 3, 2015

Closed

_config.yml name vs title #47

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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