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

Added dictionary function that could be used to pass maps into a template. #1463

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@notzippy
Contributor

notzippy commented Sep 28, 2015

Allows templates to dynamically build maps.
Example usage: Creating and passing a map with the .Site information to a subtemplate while in a range on the parent

{{$baseurl := .Site.BaseURL }}
{{range .Site.Params.Bar}}
    {{partial "foo" (dict "foo" . "BaseURL" $baseurl)}}
{{end}}
@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Sep 28, 2015

Member

Excellent idea. It would be nice with better test coverage and some lines for the documentation.

Member

bep commented Sep 28, 2015

Excellent idea. It would be nice with better test coverage and some lines for the documentation.

@notzippy

This comment has been minimized.

Show comment
Hide comment
@notzippy

notzippy Sep 29, 2015

Contributor

@bep
The test covers the two exception cases (improper number of parameters, improper type of leading parameter), and two different valid cases. What additional tests should be performed ?

For documentation do you mean for the Dictionary function itself ? I did not see any docs for any of the template functions in that file so I did not include anything. If it needs to be added I can add some..

Contributor

notzippy commented Sep 29, 2015

@bep
The test covers the two exception cases (improper number of parameters, improper type of leading parameter), and two different valid cases. What additional tests should be performed ?

For documentation do you mean for the Dictionary function itself ? I did not see any docs for any of the template functions in that file so I did not include anything. If it needs to be added I can add some..

@spf13

This comment has been minimized.

Show comment
Hide comment
@notzippy

This comment has been minimized.

Show comment
Hide comment
@notzippy

notzippy Sep 30, 2015

Contributor

Updated pull request with a description of the function

Contributor

notzippy commented Sep 30, 2015

Updated pull request with a description of the function

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Oct 1, 2015

Member

@notzippy as I read the tests, it only checks for the presence or absence of an error. There are no assertions about correct output from the success cases.

Member

bep commented Oct 1, 2015

@notzippy as I read the tests, it only checks for the presence or absence of an error. There are no assertions about correct output from the success cases.

@notzippy

This comment has been minimized.

Show comment
Hide comment
@notzippy

notzippy Oct 1, 2015

Contributor

@bep Good idea, I added an additional parameter for the expected result and checked it with a reflect.DeepEqual call on the result (when there was no error expected).

Contributor

notzippy commented Oct 1, 2015

@bep Good idea, I added an additional parameter for the expected result and checked it with a reflect.DeepEqual call on the result (when there was no error expected).

@halostatue

This comment has been minimized.

Show comment
Hide comment
@halostatue

halostatue Oct 1, 2015

Contributor

👍

Contributor

halostatue commented Oct 1, 2015

👍

if (this.expecterr && e==nil) || (!this.expecterr && e!=nil) {
t.Errorf("[%d] got an unexpected error", i, e, this.expecterr)
} else if !this.expecterr {
if !reflect.DeepEqual(r, this.expectedValue) {

This comment has been minimized.

@bep

bep Oct 2, 2015

Member

This will fail in some cases, as you cannot rely on the key order in Go maps. There are examples of ways to handle this in other tests, some less elegant than others. I believe I wrote an util func to do this, but cannot find it now.

@bep

bep Oct 2, 2015

Member

This will fail in some cases, as you cannot rely on the key order in Go maps. There are examples of ways to handle this in other tests, some less elegant than others. I believe I wrote an util func to do this, but cannot find it now.

This comment has been minimized.

@notzippy

notzippy Oct 2, 2015

Contributor

IMHO if the reflect.DeepEqual function fails to match when returned order from a slice does not match another slice then that is the fault of reflect.DeepEqual. However I am not here to argue the semantics of how DeepEqual should work :-) So I just changed it to have a single array element.

@notzippy

notzippy Oct 2, 2015

Contributor

IMHO if the reflect.DeepEqual function fails to match when returned order from a slice does not match another slice then that is the fault of reflect.DeepEqual. However I am not here to argue the semantics of how DeepEqual should work :-) So I just changed it to have a single array element.

This comment has been minimized.

@bep

bep Oct 2, 2015

Member

If you have issues with reflect.DeepEqual, take it up with the Go team. But believe me when I say this. I have been bitten by this myself. And we are not talking about slices here, but maps -- and as the Dictionary func returns a map, you will have to handle this in some way. Reducing the items in the map to 1 is hacky -- and then we are back to missing test coverage.

@bep

bep Oct 2, 2015

Member

If you have issues with reflect.DeepEqual, take it up with the Go team. But believe me when I say this. I have been bitten by this myself. And we are not talking about slices here, but maps -- and as the Dictionary func returns a map, you will have to handle this in some way. Reducing the items in the map to 1 is hacky -- and then we are back to missing test coverage.

This comment has been minimized.

@notzippy

notzippy Oct 2, 2015

Contributor

reflect.DeepEqual checks maps based on the keys https://golang.org/src/reflect/deepequal.go?#L112 , perhaps in older versions of go you may have encountered this but it is not true anymore. I can reverse the order of the map and the tests still pass.

@notzippy

notzippy Oct 2, 2015

Contributor

reflect.DeepEqual checks maps based on the keys https://golang.org/src/reflect/deepequal.go?#L112 , perhaps in older versions of go you may have encountered this but it is not true anymore. I can reverse the order of the map and the tests still pass.

This comment has been minimized.

@bep

bep Oct 2, 2015

Member

The order of the keys is not guaranteed. This has been the situation since Go 1.1. It may work 999 out of 1000 times, maybe all the time on Windows, but some times on Linux ... my point is: Having shaky tests is a bad solution. Esp. when the workaround is known: Sort the map before doing the compare.

I'm not debating this. Fix it if you want.

@bep

bep Oct 2, 2015

Member

The order of the keys is not guaranteed. This has been the situation since Go 1.1. It may work 999 out of 1000 times, maybe all the time on Windows, but some times on Linux ... my point is: Having shaky tests is a bad solution. Esp. when the workaround is known: Sort the map before doing the compare.

I'm not debating this. Fix it if you want.

This comment has been minimized.

@notzippy

notzippy Oct 8, 2015

Contributor

@bep This is the code in DeepEqual for comparing a map in deep equal it does not use the order of the keys to make the comparison, it iterates over the map keys and fetches the value per key.

        for _, k := range v1.MapKeys() {
            if !deepValueEqual(v1.MapIndex(k), v2.MapIndex(k), visited, depth+1) {
                return false
            }
        }
@notzippy

notzippy Oct 8, 2015

Contributor

@bep This is the code in DeepEqual for comparing a map in deep equal it does not use the order of the keys to make the comparison, it iterates over the map keys and fetches the value per key.

        for _, k := range v1.MapKeys() {
            if !deepValueEqual(v1.MapIndex(k), v2.MapIndex(k), visited, depth+1) {
                return false
            }
        }

This comment has been minimized.

@bep

bep Oct 9, 2015

Member

You are right. Thanks for sticking to your story. Sorry about that. If you haven't, could you "put your name" in this license thread:

http://discuss.gohugo.io/t/switching-to-apache-2-license/173

@bep

bep Oct 9, 2015

Member

You are right. Thanks for sticking to your story. Sorry about that. If you haven't, could you "put your name" in this license thread:

http://discuss.gohugo.io/t/switching-to-apache-2-license/173

This comment has been minimized.

@notzippy

notzippy Oct 9, 2015

Contributor

Cool, all good man. Have signed the release form..

@notzippy

notzippy Oct 9, 2015

Contributor

Cool, all good man. Have signed the release form..

Added dictionary function to be passed into a template.
Allows templates to dynamically build maps.
Example usage: Creating and passing a map to a subtemplate while in a range on the parent

Signed-off-by: NotZippy <notzippy@gmail.com>
@chipsenkbeil

This comment has been minimized.

Show comment
Hide comment

👍

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Oct 9, 2015

Member

Merged in 3a27cef

I modified the commit message slightly, to fit in with our (pedantic) guidelines.

This will make many users happy. And I guess it will also close some issues ... have to track those down.

Member

bep commented Oct 9, 2015

Merged in 3a27cef

I modified the commit message slightly, to fit in with our (pedantic) guidelines.

This will make many users happy. And I guess it will also close some issues ... have to track those down.

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