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

feat: lookOnePath for variant file lookup #3261

Closed
wants to merge 1 commit into from

Conversation

halostatue
Copy link
Collaborator

@halostatue halostatue commented Sep 22, 2023

This replaces code like this:

{{ $program := or (lookPath "eza") (lookPath "exa") | base }}

with code like this:

{{ $program := lookOnePath (list "era" "exa") | base }}

This changes the underlying chezmoi.LookPath to always use []string
and reduce code duplication, but the performance hit should be minimal
for normal use cases of lookPath.

Closes: #3256

This replaces code like this:

```
{{ $program := or (lookPath "eza") (lookPath "exa") | base }}
```

with code like this:

```
{{ $program := lookOnePath (list "era" "exa") | base }}
```

This changes the underlying `chezmoi.LookPath` to always use `[]string`
and reduce code duplication, but the performance hit should be minimal
for normal use cases of `lookPath`.

- One additional minor change was made to `lint-whitespace` to ignore
  `.venv` in `assets/chezmoi.io`. This change is not *yet* necessary
  for most developers, but I have been playing with using a proper venv
  for mkdocs etc., and removing and readding the venv every time is
  annoying.

Closes: twpayne#3256
@halostatue halostatue force-pushed the look-path-enhancement branch from a24359f to a57af55 Compare September 22, 2023 13:54
@twpayne
Copy link
Owner

twpayne commented Sep 25, 2023

Is this really worth a whole new template function to save a handful of characters in a template?

New template functions make sense when the functionality cannot be achieved in the template language. However, this PR introduces a new template function which wraps control flow that can be achieved in the template language.

I think

{{ $program := or (lookPath "eza") (lookPath "exa") | base }}

is already pretty concise.

@halostatue
Copy link
Collaborator Author

When there’s only two, the or is concise. When there’s more, which I have in a couple of places in my templates, it or some variant of it gets ugly and hard to read.

See:

https://github.com/halostatue/dotfiles/blob/main/home/private_dot_config/git/attributes.tmpl#L68-L70

and:

https://github.com/halostatue/dotfiles/blob/main/home/private_dot_config/git/diff-merge.tmpl#L108-L120


This is primarily useful when you have one or more programs that are functionally equivalent, even if there are slight differences in command-line parameters. For the most part, it does not matter whether you use ag, ack, pt, or rg — they all have the same basic interface. Same with exa and eza (eza is the continuation of exa). In the examples above, there are multiple ways of comparing XLS files for use with git. Some differences, yes, so I wouldn’t be able to get rid of all of the conditionals, but at that point I’m doing more explicit conditionals, IMO.

In bash, fish, and zsh it is possible to do:

$ command -v exa eza
$HOME/.cargo/bin/eza

The main difference between those and this function is that if both are found, this will only return the first:

$ command -v exa eza
$HOME/.cargo/bin/exa
$HOME/.cargo/bin/eza
$ command -v exa eza | head -1
$HOME/.cargo/bin/exa

So lookOnePath (and I don’t particularly like the name, but it matches the findOneExecutable we merged last week) is equivalent to the latter.


This could be replaced with the findOneExecutable if we had a way of getting $PATH as a list ({{ env "PATH" | splitList ":" }} only works on Unix-systems unless we have something that indicates the current path list character). Something like .chezmoi.pathList would be OK for this.

@bradenhilton
Copy link
Collaborator

This could be replaced with the findOneExecutable if we had a way of getting $PATH as a list ({{ env "PATH" | splitList ":" }} only works on Unix-systems unless we have something that indicates the current path list character). Something like .chezmoi.pathList would be OK for this.

Of course, it could always be a little nicer, but is something like this not sufficient?

{{- $pathVarSeparator := ":" -}}
{{- if eq .chezmoi.os "windows" -}}
{{-   $pathVarSeparator = ";" -}}
{{- end -}}
{{- env "PATH" | splitList $pathVarSeparator }}

@halostatue
Copy link
Collaborator Author

This could be replaced with the findOneExecutable if we had a way of getting $PATH as a list ({{ env "PATH" | splitList ":" }} only works on Unix-systems unless we have something that indicates the current path list character). Something like .chezmoi.pathList would be OK for this.

Of course, it could always be a little nicer, but is something like this not sufficient?

{{- $pathVarSeparator := ":" -}}
{{- if eq .chezmoi.os "windows" -}}
{{-   $pathVarSeparator = ";" -}}
{{- end -}}
{{- env "PATH" | splitList $pathVarSeparator }}

The point is to sort of not have this sort of noisy workaround when chezmoi actually knows or can easily know this already. Yes, I can do what I’ve already done to minimize this annoyance with my programs.tmpl and similar includable templates, but doing that feels like a poor substitute for functions.

@twpayne
Copy link
Owner

twpayne commented Sep 26, 2023

The point is to sort of not have this sort of noisy workaround when chezmoi actually knows or can easily know this already. Yes, I can do what I’ve already done to minimize this annoyance with my programs.tmpl and similar includable templates, but doing that feels like a poor substitute for functions.

I really appreciate the top quality PR and excellent discussion here. I guess I'm pushing back for a few reasons.

This "noisy workaround" only needs to be written once per user. It's probably even possible to move it into a chezmoi template with sufficient skill.

Yes, we can reduce the noise by adding a template function.

Template functions are for life. So, the bar for merging needs to be high.

We can separate functions into two classes:

  1. Those that add functionality that cannot easily be achieved in the template language itself. Many interactions with password managers are like this, especially when they involve prompting the user, setting up environments to run third-party programs with particular arguments, and caching results.
  2. Those that add control flow. or is a great example of this.

chezmoi has already accumulated multiple template functions that, with hindsight, are not necessary. Most of the JSON variants of the password manager functions are not necessary because the user can add | fromJson to their template to achieve the same effect. With hindsight, I would love to remove a lot of chezmoi's existing template functions.

Composability is key, and the more that we can separate functionality and control flow, the better. Right now lookOnePath co-mingles the two: it's effective two nested for loops (one iterating over executable names, the other iterating over a list of directories) tied into a very specific test (does this executable exist in this directory?).

This sucks for @halostatue but I think it's OK that we suffer for a bit while we find out what common patterns exist when managing dotfiles. Externals are a great example of this: it used to be that users had to use run_ scripts to download files using curl or wget, which was a total PITA. Once we'd seen enough examples of this we could come up with a built-in solution that covered the majority of use cases.

I think that we're at a similar early stage with the functionality proposed in this PR. Yes, finding varying executables across multiple directories is likely to be a common problem, but let's gather a few more use cases before committing.

@halostatue
Copy link
Collaborator Author

I am happy to let this sit for a while, but I want to respond to a few points philosophically.

The point is to sort of not have this sort of noisy workaround when chezmoi actually knows or can easily know this already. Yes, I can do what I’ve already done to minimize this annoyance with my programs.tmpl and similar includable templates, but doing that feels like a poor substitute for functions.

I really appreciate the top quality PR and excellent discussion here. I guess I'm pushing back for a few reasons.

This "noisy workaround" only needs to be written once per user. It's probably even possible to move it into a chezmoi template with sufficient skill.

This is what I have done with my map of programs (the ones linked above). Even with that, the use of that map is suboptimal because I have to remember to do toJson in the function-style template and fromJson in the caller template.

There are two main points to this from my perspective:

  1. Chezmoi does not have a wiki or much of a culture for sharing most of these sorts of snippets (specifically referring to the chezmoi.pathList idea). Discoverability of such snippets is low.
  2. Go templates are designed to make complex logic difficult and ugly to implement, because I believe that the designers intended that complex logic could be pushed into template functions. It's not quite to the extreme of moustache templates, but it is rough.

Yes, lookOnePath could be implemented as a complex snippet:

{{/* This is an included template where . is expected to be a list of wanted programs, e.g. `(list "eza" "exa")` */}}
{{- $result := '' -}}
{{- range $value := . -}}
{{-   if and (not $result) ($entry := lookPath .) -}}
{{-    $result = $entry -}}
{{-  end -}}
{{- end -}}
{{- $result -}}

But if I implement that, where do I share it? Yes, we have the show and tell discussion, but there are twelve posts there. It's not necessarily an easy thing to understand, use, or explain because it's a function-as-a-template.

Template functions are for life. So, the bar for merging needs to be high.

chezmoi has already accumulated multiple template functions that, with hindsight, are not necessary. Most of the JSON variants of the password manager functions are not necessary because the user can add | fromJson to their template to achieve the same effect. With hindsight, I would love to remove a lot of chezmoi's existing template functions.

I disagree that it need be "for life". While I would not want to remove template functions in a minor version, I think that template functions should be considered carefully as part of #2673. It is an opportunity to rectify some items (like maybe replacing sprig with an in-house library and consolidating that functionality to be better suited for chezmoi). If we are open to adding deprecation messages (I have some thoughts, but they are nascent), then I think that the current onepassword* template functions should be replaced with something that more closely matches the native output of 1Password CLI v2. And there are other template functions as well that should be reconsidered this way.

If 1Password went out of business tomorrow, the presence or absence of the onepassword* template functions would not matter because op would no longer work, and people would have to deal with an alternative approach anyway.

With sufficient warning and built-in replacements, I see no reason why the as-yet-unscheduled chezmoi 3 can't remove or rename some templates (we would want to discourage auto-update from 2.x to 3.x if we do this, though).

Composability is key, and the more that we can separate functionality and control flow, the better. Right now lookOnePath co-mingles the two: it's effective two nested for loops (one iterating over executable names, the other iterating over a list of directories) tied into a very specific test (does this executable exist in this directory?).

Yes and no. That is much more what findOneExecutable does (findOneExecutable (list needle...) (list haystack...)), whereas lookOnePath (list needle...) is a repeated short-circuiting cached alternative to the possible (untested) example I gave above.

I honestly think that the biggest problem with this function and findOneExecutable is the name. I’d be happy to take findOneExecutable out of the linked navigation in the docs so that we can find a better name for that; it might be sufficient if we have a better name and possibly a better design.


Thinking about a possibly better design for findOneExecutable (findFirstCommand? — to have a parallel to command -v in shells):

  • The current API is findOneExecutableTemplateFunc(needles, haystack []string) string.
  • If we change it to findOneExecutableTemplateFunc(needles []string, haystack any) string — but effectively have it as findOneExecutableTemplateFunc(needles []string, haystack null | []string) — where null (missing optional last parameter) is treated as $PATH / %PATH% split appropriately — then I think we can have just one function for this that works both ways.
  • I would not suggest a similar function merging for lookPath / findExecutable (but I would choose lookPath if we were to do this), mostly because it would be unnecessary and complicate the caching further than what this PR complicates things.

I think that I’m pushing for alternatives on this still in part because I think that having to have the templates figure out what Go already knows (the path list separator) is what bothers me most about @bradenhilton's suggested template. IMO, template functions should be about what you as the manager of your dotfiles know, not what Go knows.

@bradenhilton
Copy link
Collaborator

Template functions are for life. So, the bar for merging needs to be high.
...
chezmoi has already accumulated multiple template functions that, with hindsight, are not necessary.

@twpayne while I agree with the general sentiment, I disagree that template functions are for life. For example, the ioreg function has been deprecated and marked for removal in a future version of chezmoi.

I think the eqFold function, which I wrote (with help, which was greatly appreciated!) only just meets this bar, as there doesn't seem to be a trivial way to perform unicode comparisons of strings. To be honest though, converting strings to the same casing and comparing them would cover the overwhelming majority of use cases.

I think v3 gives us a great opportunity to rectify a lot of this. We can get chezmoi/templatefuncs up to par with sprig (which I personally don't mind focusing on soon, as many function implementations are fine to copy as is, just with a more pipe-friendly parameter order).

Slightly off-topic, but I personally think we should remove explicit JSONC support (fromJsonc etc.) and just bake it into the main JSON support. My understanding is that JSONC parsers only add the ability to ignore or remove comments before parsing the rest as regular JSON. A lack of comments would not break this, so it should be fine to invariably parse all JSON as if it were JSONC.

@twpayne
Copy link
Owner

twpayne commented Nov 14, 2023

@halostatue what are your current thoughts on this PR?

@halostatue
Copy link
Collaborator Author

I will need to revisit it and don't have time this week. I still don't think it's a bad idea, but some of the need has been removed by the merge of #3270 and the previous merge of #3260.

@halostatue
Copy link
Collaborator Author

Closing for now. I may revisit this in the future as lookPaths (note the plural) that would return []string or nil in the same way that command -v eza exa will return both paths if both are installed. It would still mean some post-processing, but it would make this "alternatives" approach easier overall.

@twpayne twpayne closed this Nov 27, 2023
@halostatue halostatue deleted the look-path-enhancement branch December 14, 2023 01:23
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 8, 2024
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.

Look for one of several executables in $PATH
3 participants