-
Notifications
You must be signed in to change notification settings - Fork 502
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
Conversation
fd8cf58
to
3345dee
Compare
3345dee
to
a24359f
Compare
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
a24359f
to
a57af55
Compare
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
is already pretty concise. |
When there’s only two, the See: and: 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 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 This could be replaced with the |
Of course, it could always be a little nicer, but is something like this not sufficient?
|
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 |
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:
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 Composability is key, and the more that we can separate functionality and control flow, the better. Right now 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 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. |
I am happy to let this sit for a while, but I want to respond to a few points philosophically.
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 There are two main points to this from my perspective:
Yes,
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.
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 If 1Password went out of business tomorrow, the presence or absence of the 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).
Yes and no. That is much more what I honestly think that the biggest problem with this function and Thinking about a possibly better design for
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. |
@twpayne while I agree with the general sentiment, I disagree that template functions are for life. For example, the I think the I think v3 gives us a great opportunity to rectify a lot of this. We can get Slightly off-topic, but I personally think we should remove explicit JSONC support ( |
@halostatue what are your current thoughts on this PR? |
Closing for now. I may revisit this in the future as |
This replaces code like this:
with code like this:
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