Open twpayne opened 1 year ago
Perhaps we could also take the opportunity to implement a higher-order function (which would add to the funcMap
, not something done at runtime) similar to the one in this issue?
As you know, there is the quoteList
template function, which was implemented explicitly. So far, there hasn't been much of a demand sufficient to add similar functions, which would probably be necessary to add an explicit function (and thus create clutter and perhaps some complexity).
An each
function separates the iteration logic from the action being taken on the item in the list. Going forward, implementing stringFuncEach
would likely be as simple as implementing stringFunc
and then setting funcMap["stringFuncEach"]
to something like eachFunc(stringFunc)
.
The each{Func}
functions could be as generic as possible or type specific if needed.
Maybe this could even be done dynamically with reflection? So we could do something like:
funcMap[funcName + "Each"] = eachFunc{Type}(func)
(probably not syntactically correct)Separating the logic would remove the need for explicit functions in all but the most specialized cases, which would remove clutter and be trivial to extend as new functions are added.
The alternative, which would be better, but also likely increase complexity, would be to add iteration logic to every requisite function, so that you can pass it a single item or a list of items and get the expected output, e.g.
$string | quote
"foobarbaz"
$list | quote
["foo", "bar", "baz"]
This approach would be easier if we had control over how each function is implemented, as sprig is essentially a black box in this case.
Likely v3 because this would mean removing/replacing/renaming quoteList
at the very least.
Interested in any thoughts anyone might have.
Perhaps we could also take the opportunity to implement a higher-order function
The tricky thing here is that text/template
is, by design, a simple language and does not support higher-order functions, and is unlikely to ever do so in the future. So, without permanently forking text/template
(which would be an immeasurable burden), this is not possible to do.
Doesn't it just add function permutations to the funcMap
?
I know it's probably not possible to do something like
{{ $list | map | squote }}
but that is different, no?
I tried dropping the example in the linked issue in to test but it's not compatible without some tweaks (...interface{}
vs []string
etc.) and I ran out of time that day so I just left it there.
Are you saying it's impossible to do something like
funcMap["squoteEach"] = each(squote)
funcMap["add1Each"] = each(add1)
without a text/template
fork?
Making all template functions list-aware somehow is probably more user friendly anyway, but I'm not sure exactly what that implementation would look like.
Should we create a funcs
repo in the chezmoi organization?
Ah, sorry I misunderstood. My misunderstanding was that I thought that each
was to be a template function that took a function and a list as arguments and returned a new list of the function applied to each element in the list. This isn't possible with text/template
as text/template
does not support functions as values.
If I now I understand correctly, each
should be a Go function that makes it easier to write template functions that apply a Go function to every element in a list. This is certainly possible, and doesn't need to wait for v3 as it's an internal implementation detail that is not visible to users (with the exception of a few more template functions being added).
I have to ask, though: given that text/template
already supports range
/end
actions, how much are these extra functions really needed? quoteList
is certainly useful as you often need to quote multiple strings for command arguments, but what other functions do you anticipate being regularly useful?
Note it's also possible to extend template functions so that they operate on lists as well as isolated values. For example, we can extend quote
to return a list of quoted strings if it is passed a list of strings, without breaking its existing functionality of returning a quoted string if passed a single string.
Should we create a
funcs
repo in the chezmoi organization?
Hmm, yes. I'll create a templatefuncs
repo with some initial ideas.
Note it's also possible to extend template functions so that they operate on lists as well as isolated values.
I think we should just do this. I'd argue that {{ $list | quote }}
implicitly iterating through the list and quoting each element is expected behavior (as opposed to something like implicitly joining the elements, then quoting) and doesn't increase the number of template functions.
So, sadly, thanks to Masterminds/sprig's inherent brokenness, this isn't as immediately fixable as I had hoped. Some specific problems:
quote
function already accepts lists ... and quotes them:
$ chezmoi execute-template '{{ list "a" "b" | quote }}'
"[a b]"
so changing quote
's behavior for lists will be a backwards-incompatible change.
squote
. It's not even slightly correct: strings with a single quote in them are not correctly escaped 🤯 .So, replacing the template library is definitely something for chezmoi v3, as trying to maintain backwards compatibility with all of Minorminds stupidity is just not possible.
Also, just to clarify as I didn't do it before...
I have to ask, though: given that
text/template
already supportsrange
/end
actions, how much are these extra functions really needed?
You're absolutely right, *especially if the solution was to add `Eachpermutations of each function to the
funcMap**, but if we control the implementations of the functions this is a non-issue, as we can make them list-aware. This has some advantages over
range/
end`:
Improved template readability. range
/end
require whitespace management. Often I want to do something like this:
{{ range $list }}
{{ . | quote }}
{{ end }}
for readability, but the whitespace is included in the output, so it must be removed with the control characters. This is one of the simplest examples I could come up with and it's still not easy to reason about at a glance in my opinion.
Also, it's not like this approach would become unusable, it would still work perfectly as a backup or a preference depending on the user.
I'm not sure if I can articulate this properly, but this approach is also entirely focused on the output, it doesn't affect the $list
variable in any way, so it's difficult to pipe the result to something else. Speaking of which...
Improved pipelining. This removes the need for temporary variables/variable re-assignment/multi-line actions etc., which should allow something like the following:
{{ $numList | add1 | quote }} # here I am assuming that the functions work correctly with lists
https://github.com/chezmoi/templatefuncs contains some initial replacement template functions. It's a work in progress.
I asked on golang-nuts if there are existing alternatives to Minorminds/sprig.
I was looking at gomplate the other day and it seems to me that it has a few nice things about it and may be a viable replacement for sprig in a number of places; it has other features that aren't as applicable to Chezmoi (but many of them would be, I think).
The biggest thing that I liked about it is that it exposes objects rather than functions (although some functions are common enough to expose aliases), so for v3 (#2673) it would be possible to have onepassword.Details
instead of onepasswordDetails
&c.
@halostatue I'd personally rather take inspiration from gomplate than integrate it into chezmoi.
If gomplate went the way of sprig, we would just find ourselves in this situation again. Creating our own bespoke function library allows us to fix and extend things quickly, rather than having to patch in our own fixes and create discrepancies like the one mentioned recently in https://github.com/twpayne/chezmoi/issues/3335:
On a related note,
toPrettyJson
is listed on chezmoi's website, which suggests that you're rolling your own implementation instead of just using sprig's. If that suggestion is false then I recommend removing that page.
Using objects just isn't necessary for some use cases. Would we really benefit from having something like strings.ToUpper
instead of toUpper
? The aliases would just create clutter in the FuncMap. You also end up with awkward names like conv.ToString
and coll.Dict
to avoid having the names be too verbose.
For some operations, sure having both the alias and the full version would be a waste. On the other hand, collecting password manager functionality into objects with functions available would IMO be much cleaner.
Providing migration steps for this would be vital (e.g., reimplement the 1Password functionality as onepassword.*
or op.*
, but put the aliases in place; start warning about the use of those aliases in a future release, and remove them at Chezmoi 3).
The warning phase could even be done as "light" (the use of a deprecated function/alias warns once) and "heavy" (each use of a deprecated function/alias warns).
This sort of approach would allow us to think about what functions should be quickly available (toString
, toUpper
, toJson
, fromJson
, etc.) and what functions should be grouped together.
I may put together a list of the functions available in chezmoi via sprig or direct and suggest some sort of organization.
Generally I agree, but I think we need to get https://github.com/chezmoi/templatefuncs in a much more complete state before we even think about that.
I think v3 should be a clean break, and we should be as noisy as would be reasonable about that leading up to its release (once the new features are nearing completion).
So, I propose to make this change in an incremental way, without needing chezmoi v3, with the following strategy:
template.functions
config variable which is a list of strings in priority order.github.com/masterminds/sprig
("sprig") corresponds to the existing minorminds/sprig functions and the list element github.com/chezmoi/templatefuncs
corresponds to https://github.com/chezmoi/templatefuncs ("templatefuncs").template.functions
will be github.com/masterminds/sprig
.This should allow an orderly transition from sprig to templatefuncs, and of course the user can use chezmoi diff
and chezmoi's error reporting to catch any problems along the way. As template.functions
will be set in the user's configuration file, they can even use different template functions on different systems (if they are mad enough).
@bradenhilton thoughts?
This seems like a reasonable approach to me.
@twpayne How do you propose we handle the current chezmoi-specific functions, especially functions such as ioreg
and our overridden sprig functions? My understanding was that ioreg
would be removed, and the rest would eventually be rolled into templatefuncs when ready, but allowing templatefuncs now complicates that slightly.
I do think incremental adoption is a good idea.
Just came across this fork of sprig: https://github.com/go-sprout/sprout
Here's an issue with some discussion on roadmap: https://github.com/go-sprout/sprout/issues/1 More discussion on improvements here: https://github.com/orgs/go-sprout/discussions
I do not know if sprout aims to solve the problems raised in the first post of this issue. This is just a FYI :)
Hi everyone, as currently the main maintainer of go-sprout/sprout, I wanted to provide some additional context and insights regarding your initial issue.
The argument order of its functions is incompatible with Go template's pipelining
Firstly, I agree this is something we should address in order to improve compatibility.
Its naming convention does not match Go's
Secondly, the naming convention of sprig does not match Go's, which could lead to confusion or issues when using the library. Additionally, I'd like to highlight the aliasing function feature, we use internally for backward-compatibility reasons. You can learn more about this by checking out the alias.go file on our repository (https://github.com/go-sprout/sprout/blob/main/alias.go#L9-L46).
Giving a second life to sprig with sprout is an exciting challenge due to who use sprig (like Helm, tempo and Argo), we need to involve with backward-compatibility to don't break end-users configuration and go forward too.
This is a complex issue, but I'm confident we can find a solution that balances both needs.
If you have any questions or concerns, please don't hesitate to reach out on our discussions page (https://github.com/orgs/go-sprout/discussions) or ping me directly (@42atomys).
Have a great day everyone!
Is your feature request related to a problem? Please describe.
github.com/masterminds/sprig
, used by chezmoi, has unfortunately become a popular library of template functions despite several critical issues:toJson
should betoJSON
).Describe the solution you'd like
chezmoi's template functions should be up-to-date, compatible with Go template pipelining, and match Go's naming conventions.
Describe alternatives you've considered
Keeping minorminds/sprig template functions as-is.
Additional context
A smooth transition should be provided, with both old and replacement template functions being available to users for some time. Users can use
.chezmoiversion
to help the transition.