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

I think v needs to be in a closure here #48

Open
bennidhamma opened this issue Apr 12, 2017 · 1 comment
Open

I think v needs to be in a closure here #48

bennidhamma opened this issue Apr 12, 2017 · 1 comment

Comments

@bennidhamma
Copy link

I've made a few hacks on this code to support loading project.json from an input parameter. (it's super crude - I have never worked with Go before, but if you like you can see my hatchet job here: https://github.com/themaven-net/boilr).

In the process, I saw a strange bug where things would work normally if I went through the prompts, but if I used defaults, then all the replacements were ending up being the same value. This has the smell of a closure-related bug all over it, and I seemed to find a spot where we are failing to enclose some key variables:

if t.ShouldUseDefaults {
   t.FuncMap[s] = func() interface{} {
	switch v := v.(type) {
	// First is the default value if it's a slice
	case []interface{}:
		return v[0]
	}
	return v
    }
} else {
        t.FuncMap[s] = prompt.New(s, v)
}

Notice how we put s and v into a closure when we invoke prompt.New, but we don't do it for the shouldUseDefaults case.

I ended up "fixing it" by putting the funcMap[s] assignment into a closure:

if t.ShouldUseDefaults {
      t.FuncMap[s] = func(s2 string, v2 interface{}) func() interface{} {
	return func() interface{} {
	  switch v2 := v2.(type) {
	  // First is the default value if it's a slice
	  case []interface{}:
	    tlog.Error(fmt.Sprintf("s: %v, v[0]: %s\n", s2, v2[0]))
	    return v2[0]
	  }
	  tlog.Error(fmt.Sprintf("s: %v, %v \n", s2, v2))
	  return v2
	}
      }(s, v)
    } else {
      t.FuncMap[s] = prompt.New(s, v)
    }

And now it works correctly for me. This is essentially what the prompt (non-default) mode is doing. It makes sense to me that we would need to retain s and v inside of a closure for the defaults like we do for the prompt path. I don't understand why the change I introduced would have caused this, but I can't imagine I'm the first person to see this behavior if it is indeed a bug.

Any idea what I'm not seeing? I can live with this, but it's definitely puzzling.

@Ilyes512
Copy link

Oeps, should have checked issues first :P #74

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants