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

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

Closed
wants to merge 1 commit into from

Conversation

notzippy
Copy link
Contributor

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
Copy link
Member

bep commented Sep 28, 2015

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

@notzippy
Copy link
Contributor Author

@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
Copy link
Contributor

spf13 commented Sep 30, 2015

@notzippy
Copy link
Contributor Author

Updated pull request with a description of the function

@bep
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor

👍

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) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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
Copy link

👍

@bep
Copy link
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.

@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 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants