Skip to content

tpl: Add default function #1943

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

tpl: Add default function #1943

wants to merge 5 commits into from

Conversation

@moorereason
Copy link
Contributor

No description provided.

@moorereason moorereason changed the title tpl: Add default function Mar 9, 2016
@digitalcraftsman
Copy link
Member

👍
@moorereason
Copy link
Contributor Author

Hold up. I found an issue that I need to fix.
@digitalcraftsman
Copy link
Member

I didn't planned to merge anything myself 😄
bep
bep reviewed Mar 9, 2016
View reviewed changes
tpl/template_funcs_test.go
func TestDefault(t *testing.T) {
for i, this := range []struct {
dflt interface{}
given interface{}
Copy link
Member

Choose a reason for hiding this comment

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

Need nil-variants for these.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.
@moorereason
tpl: fix default function
018bb35 
This commit fixes a few things:

1. `given` is now a variadic parameter so that piping works properly
2. add separate template tests to make sure piping works
3. support time values
4. add more tests of the dfault function
@moorereason
Copy link
Contributor Author

I had to rework the func signature to use a variadic parameter for given; otherwise, the following construct would given an error about argument count mismatch when the key was undefined:

{{ index . "key" | default "foo" }}

Apparently, the Go template engine doesn't pass a nil(?) value through the pipe.

I can rebase and squash if this looks good to everyone.
@moorereason moorereason changed the title WIP: tpl: Add default function Mar 9, 2016
@bep
Copy link
Member

bep commented Mar 10, 2016

Looks mostly good, but I have one question:

{map[string]interface{}{"images": []string{}}, `{{ default "default.jpg" (index .images 0) }}`, `default.jpg`, true},

The above testcase fails. And while I understand why it fails, it is confusing for the users that this works for maps, but not for slices. I use @derekperkins "list of images" syntax, and then have a rather clumsy construct to pick the first if found. I would rather use default.
@bep
Copy link
Member

bep commented Mar 10, 2016

On an added note: I don't expect you to "hook into" the Golang internals to get this to work. I don't see how. But I'm just thinking out loud: If we could override the index func somehow, I think it would be better for Hugo's use case if it returned nil and not index out of range error in this case.
@bep bep mentioned this pull request Mar 10, 2016
Closed
@moorereason
Copy link
Contributor Author

@bep,
I have an overloaded index staged in the branch used by this PR. It works with your test case above. Can I push it with this PR or do you want a separate PR?
@bep
Copy link
Member

bep commented Mar 10, 2016

@moorereason push it with this PR, add my test case + reference the issue I just created.
@moorereason
tpl: Add custom index function
e8032fa 
This commit adds a custom index template function that deviates from the stdlib
simply by not returning an "index out of range" error if an array, slice or
string index is out of range.  Instead, we just return nil values.  This should
help make the new default function more useful for Hugo users.

Fixes gohugoio#1949
@moorereason
Copy link
Contributor Author

Already pushed. 😄 I did pretty much everything you mention except I didn't leave the original code there as a comment; however, I did leave a comment about the deviation from stdlib and reference where I copied it from (there were 3 funcs total).
@bep
Copy link
Member

bep commented Mar 10, 2016

It looks good. It just have to be obvious what to replace/change if they fix this at some point in the stdlib. I will have a look at and merge this later.
@bep
Copy link
Member

bep commented Mar 10, 2016

Merged in 2d0650d Good work.
@bep bep closed this Mar 10, 2016
@rdwatters
Copy link
Contributor

👍🏻👍🏻👍🏻
@moorereason
Copy link
Contributor Author

@bep, this PR was never merged. What was merged was a custom index implementation that makes the default implementation more useful.

Reopening and will modify the treatment of handling of booleans based on #1953.
@moorereason moorereason reopened this Mar 16, 2016
@moorereason
Copy link
Contributor Author

Nevermind, this was merged. I should sleep more.

/me walks away in shame.
@moorereason moorereason deleted the default branch March 16, 2016 19:58
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Enhancement

4 participants

@moorereason @digitalcraftsman @bep @rdwatters