Adds lookPathIn

[arran4]

Resolves: #3141

As per: #3141

Adds lookPathIn

Not sure the testing procedure. Followed: https://www.chezmoi.io/developer/contributing-changes/

Microcommiters hate: https://www.conventionalcommits.org/en/v1.0.0/#summary -_-

[arran4]

@twpayne Need to figure out how to make this work on windows too.

[arran4]

@twpayne Please approve running now.

[arran4]

@twpayne Okay I did what I could and I'm probably going to abandon efforts to get the windows test working. The unit tests show that it does work, there is just something odd about the txtar which is causing an issue.

[halostatue]

This is good work, and I love the idea of this function, and I’m more than happy to give a detailed review, but… Right now I don’t love this function, and many of the comments that I would make would be covered by the concerns from this. In particular, I don’t love the fact that it’s expecting an OS-specific PATH list. (As an aside, that's a {,ba,z,k}sh-specific thing. In Fish, $PATH is a list, not a colon-separated list.)

{{ $found := lookPathIn "something" "foo:bar:baz" }}
{{/* On Windows */}
{{ $found := lookPathIn "something" "foo;bar;baz" }}

I think that we should either embrace a variadic form (lookPathInTemplateFunc(needle string, ...haystack string) string), an explicit array form (lookPathInTemplateFunc(needle string, haystack string[]) string), or something more dynamic and in-between (lookPathInTemplateFunc(needle string, ...haystack any) string).

These would allow:

{{ $found := lookPathIn "something" "foo" "bar" "baz" }}{{/* variadic, dynamic */}}
{{ $found := lookPathIn "something" (list "foo" "bar" "baz") }}{{/* array, dynamic */}}

The hard part about the dynamic form would be the error handling if given a non-string or non-string-array. Other questions about the dynamic form:

• would we allow mixed strings and string-arrays?
• would we base our assumption of what the rest of the values are based on the first value?

With the way it’s implemented now, I think that it’s really confusing — and you’re going to end up calling file.splitList anyway, so why force people to join it in a platform-specific way just to split it apart again later? The list character is also not exposed in any way to chezmoi templates, so that becomes knowledge that people Just Have To Know (that might be a useful function joinOsList or something, but…).

My preference is for dynamic, either all strings or one array (which still requires ...haystack any), because that allows me to build a list and then look, or to specify a list as parameters. If we don’t want the complexity of dynamic resolution, then I would make it explicitly two parameters (lookInPath "needle" (list "straw" "straw" "straw")) for an array.

What do you guys think?

[arran4]

I think if you're not assuming a platform standard it should potentially be something else. The value add are the assumptions as you take those assumptions away the work involved to use it increases. While it's a good point it doesn't help with fish I'm not sure what we can do in the scope of this function. I feel to match yours a separate feature set (if it doesn't already exist) is required to add functional programming like operators and expose SplitList if it hasn't already

[halostatue]

I think if you're not assuming a platform standard it should potentially be something else. The value add are the assumptions as you take those assumptions away the work involved to use it increases. While it's a good point it doesn't help with fish I'm not sure what we can do in the scope of this function. I feel to match yours a separate feature set (if it doesn't already exist) is required to add functional programming like operators and expose SplitList if it hasn't already

But this function isn’t using PowerShell, bash, fish, or cmd to find executables, it’s using a customized copy of a Go builtin to search a list of paths…that we have to split because we are requiring that it be joined in the first place. It’s better to think of this outside of the OS box and instead think of it in terms of what we want to do and how we want to use this. I, for one, do not want to use this by having to remember to join with ":" or ";" depending on platform, and it feels like a usability regression over running chezmoi twice at that point.

• The easiest thing for simple use would be to accept ...haystack string, because it’s then {{ lookPathIn "needle" "straw" "straw" "straw" }}.
• The easiest thing for template reuse would be to accept haystack []string, because it’s then {{ lookPathIn "needle" $haystack }} (where $haystack is a previously created list).

We can do both, but that slightly increases the lookPathInTemplateFunc definition complexity:

func lookPathInTemplateFunc(needle string, haystack ...any) string {
  if len(haystack) == 0 {
    panic(fmt.Errorf("expected at least one haystack parameter"))
  }

  switch haystack[0].(type) {
    case string:
      return chezmoi.lookPathIn(needle, haystack)
    case []string:
      return chezmoi.lookPathIn(needle, haystack[0])
    default:
      panic(fmt.Errorf("invalid haystack type, must be string or []string"))
  }
}

At that point, there’s no need for joining, no need to expose what the list character is, and no need for chezmoi.lookPathIn to split the characters. It’s also completely shell and platform independent from the template user perspective, which is the main thing that I think matters.

[twpayne]

I agree that it makes more sense for the haystack to be a list rather than a string. chezmoi already has a splitList template function. Accepting exactly one haystack parameter is also fine: there are several template functions for combining lists if needed.

[arran4]

I keep forgetting the sprig reexport exists. I will create new commits in new PRs for the broken up functions. The name of the function would not be appropriate as it's really just a minimal modification of the lookPath re-exported function allowing you to provide your own. So I will choose a different name. I am thinking; findExecutableByName or something along those lines. Let me know if you have any opinions.