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

tpl: Add default function #1943

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
4 participants
@moorereason
Contributor

moorereason commented Mar 9, 2016

No description provided.

@moorereason moorereason changed the title from tpl: Add default function to WIP: tpl: Add default function Mar 9, 2016

@digitalcraftsman

This comment has been minimized.

Show comment
Hide comment
Member

digitalcraftsman commented Mar 9, 2016

👍

@moorereason

This comment has been minimized.

Show comment
Hide comment
@moorereason

moorereason Mar 9, 2016

Contributor

Hold up. I found an issue that I need to fix.

Contributor

moorereason commented Mar 9, 2016

Hold up. I found an issue that I need to fix.

@digitalcraftsman

This comment has been minimized.

Show comment
Hide comment
@digitalcraftsman

digitalcraftsman Mar 9, 2016

Member

I didn't planned to merge anything myself 😄

Member

digitalcraftsman commented Mar 9, 2016

I didn't planned to merge anything myself 😄

Show outdated Hide outdated tpl/template_funcs_test.go
func TestDefault(t *testing.T) {
for i, this := range []struct {
dflt interface{}
given interface{}

This comment has been minimized.

@bep

bep Mar 9, 2016

Member

Need nil-variants for these.

@bep

bep Mar 9, 2016

Member

Need nil-variants for these.

This comment has been minimized.

@moorereason

moorereason Mar 9, 2016

Contributor

Fixed.

@moorereason

moorereason Mar 9, 2016

Contributor

Fixed.

Show outdated Hide outdated docs/content/templates/functions.md
Checks whether a given value is set and returns a default value if it is not.
"Set" in this context means true for booleans; non-zero for numeric types;
non-zero length for strings, arrays, slices, and maps; any struct value; or
non-nil for any other types.

This comment has been minimized.

@bep

bep Mar 9, 2016

Member

I think you should add Zero for time.Time (time.IsZero) to the list of not set.

@bep

bep Mar 9, 2016

Member

I think you should add Zero for time.Time (time.IsZero) to the list of not set.

This comment has been minimized.

@moorereason

moorereason Mar 9, 2016

Contributor

Good idea.

@moorereason

moorereason Mar 9, 2016

Contributor

Good idea.

This comment has been minimized.

@moorereason

moorereason Mar 9, 2016

Contributor

Fixed.

@moorereason

moorereason Mar 9, 2016

Contributor

Fixed.

tpl: fix default function
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

This comment has been minimized.

Show comment
Hide comment
@moorereason

moorereason Mar 9, 2016

Contributor

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.

Contributor

moorereason commented Mar 9, 2016

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 from WIP: tpl: Add default function to tpl: Add default function Mar 9, 2016

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Mar 10, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@bep

bep Mar 10, 2016

Member

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.

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.

@moorereason

This comment has been minimized.

Show comment
Hide comment
@moorereason

moorereason Mar 10, 2016

Contributor

@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?

Contributor

moorereason commented Mar 10, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@bep

bep Mar 10, 2016

Member

@moorereason push it with this PR, add my test case + reference the issue I just created.

Member

bep commented Mar 10, 2016

@moorereason push it with this PR, add my test case + reference the issue I just created.

tpl: Add custom index function
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 #1949
@moorereason

This comment has been minimized.

Show comment
Hide comment
@moorereason

moorereason Mar 10, 2016

Contributor

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).

Contributor

moorereason commented Mar 10, 2016

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

This comment has been minimized.

Show comment
Hide comment
@bep

bep Mar 10, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@bep

bep Mar 10, 2016

Member

Merged in 2d0650d Good work.

Member

bep commented Mar 10, 2016

Merged in 2d0650d Good work.

@bep bep closed this Mar 10, 2016

@rdwatters

This comment has been minimized.

Show comment
Hide comment
@rdwatters

rdwatters Mar 11, 2016

Contributor

👍🏻👍🏻👍🏻

Contributor

rdwatters commented Mar 11, 2016

👍🏻👍🏻👍🏻

@moorereason

This comment has been minimized.

Show comment
Hide comment
@moorereason

moorereason Mar 16, 2016

Contributor

@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.

Contributor

moorereason commented Mar 16, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@moorereason

moorereason Mar 16, 2016

Contributor

Nevermind, this was merged. I should sleep more.

/me walks away in shame.

Contributor

moorereason commented Mar 16, 2016

Nevermind, this was merged. I should sleep more.

/me walks away in shame.

@moorereason moorereason deleted the moorereason:default branch Mar 16, 2016

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