feat: add more list funcs by bradenhilton · Pull Request #29 · chezmoi/templatefuncs

bradenhilton · 2024-05-18T12:47:03Z

WIP

bradenhilton · 2024-05-18T15:51:54Z

I'm struggling a bit with trying to implement a sort function.

I wanted to use slices.Sort, but I don't know how to properly constrain the type of the input.

Obviously, if a slice contains elements that are different types, it cannot be sorted in this way, so I would need to somehow check if every element in the slice is the same type, and that type implements cmp.Ordered.

I've tried a type switch, and I've tried reflection, but it always goes to the default case and just returns the existing list.

bradenhilton · 2024-05-18T16:57:07Z

We could create a []string copy of the input, sort that, then apply the index changes to the input (or a copy) and return it, but I'm not sure if that has any edge cases at the moment.

bradenhilton · 2024-05-18T19:22:55Z

+// isZeroValue returns whether a value is the zero value for its type.
+// An empty array, map or slice is assumed to be a zero value.
+func isZeroValue(v any) bool {
+	vval := reflect.ValueOf(v)
+	if !vval.IsValid() {
+		return true
+	}
+	switch vval.Kind() { //nolint:exhaustive
+	case reflect.Array, reflect.Map, reflect.Slice, reflect.String:
+		return vval.Len() == 0
+	case reflect.Bool:
+		return !vval.Bool()
+	case reflect.Complex64, reflect.Complex128:
+		return vval.Complex() == 0
+	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
+		return vval.Int() == 0
+	case reflect.Float32, reflect.Float64:
+		return vval.Float() == 0
+	case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
+		return vval.Uint() == 0
+	case reflect.Struct:
+		return false
+	default:
+		return vval.IsNil()
+	}
+}
+


Could this be replaced with something like

truth, ok := template.IsTrue(v) if !ok { panic(fmt.Sprintf("unable to determine zero value for %v", v)) } return !truth

?

twpayne

Thanks for this, but there are several problems. See the individual review comments.

The underlying problem is that we're trying to build dynamically-typed template functions on top of strict, statically-typed Go functions, and an extra conversion layer is needed.

twpayne · 2024-05-20T08:15:19Z

+If *list* cannot be sorted, it is simply returned.
+
+```text
+{{ list list "c" "a" "b" | sort }}


nit: duplicated list.

twpayne · 2024-05-20T08:16:52Z

 func NewFuncMap() template.FuncMap {
 	return template.FuncMap{
+		"compact":          compactTemplateFunc,
+		"concat":           slices.Concat[[]any],


This needs to be more generic so that it can accept []string and other slices. Note that Go cannot automatically convert []any to []string. See https://blog.merovius.de/posts/2018-06-03-why-doesnt-go-have-variance-in/ if you want the gory details.

twpayne · 2024-05-20T08:17:06Z

 		"contains":         reverseArgs2(strings.Contains),
 		"eqFold":           eqFoldTemplateFunc,
 		"fromJSON":         eachByteSliceErr(fromJSONTemplateFunc),
+		"has":              reverseArgs2(slices.Contains[[]any]),


Same comment as for concat.

twpayne · 2024-05-20T08:18:49Z

+	case reflect.Uint:
+		return convertAndSortSlice[uint](list)
+	case reflect.Uint8:
+		return convertAndSortSlice[uint8](list)


This will not work as expected, as []byte is an alias for []uint8 and in other template functions we treat []bytes as strings.

twpayne · 2024-05-20T08:19:20Z

+		}
+	}
+
+	switch firstElemType.Kind() { //nolint:exhaustive


No need to be exhaustive here. It's sufficient to cover the common types (int, float64, string, and bool).

If I don't disable exhaustive the linter complains because I'm not accounting for the other Kind cases:

missing cases in switch of type reflect.Kind: reflect.Invalid, reflect.Bool, reflect.Complex64, reflect.Complex128, reflect.Array, reflect.Chan, reflect.Func, reflect.Interface, reflect.Map, reflect.Pointer|reflect.Ptr, reflect.Slice, reflect.Struct, reflect.UnsafePointer (exhaustive)

You can use a default: case which panics with an "unsupported type" message.

I'm already using a default case, though.

Granted, it doesn't panic, but I just changed it and it also doesn't satisfy the linter:

Hmm, in that case feel free to disregard/disable the linter. Note that this exhaustive checking might be covered by multiple linters and you might need to disable all of them. Also, check to see if the warning is coming from golangci-lint with chezmoi's configuration, or if it's an extra linter being run by your IDE.

https://pkg.go.dev/github.com/nishanths/exhaustive#hdr-Definition_of_exhaustiveness

We can add the -default-signifies-exhaustive flag to the linter if you'd prefer.

Also, I'm using golangci-lint as the linter in VS Code with Error Lens, as well as Run on Save to format:

{ "go.lintTool": "golangci-lint", "go.lintFlags": ["--fast"], "emeraldwalk.runonsave": { "commands": [ { "match": "\\.go$", "cmd": "gci write ${file} --skip-generated -s standard -s default -s prefix(github.com/chezmoi/templatefuncs)" }, { "match": "\\.go$", "cmd": "golines --base-formatter=\"gofumpt -extra\" --max-len=128 --write-output ${file}" } ] } }

twpayne · 2024-05-20T08:20:28Z

+}
+
+// sortTemplateFunc is the core implementation of the `sort` template function.
+func sortTemplateFunc(list []any) any {


Trying to write a generic sort function is hard. I would instead have separate sortInts and sortFloat64s functions, with sort converting all arguments to strings and then sorting those.

Is what I described as terrible sort roughly sufficient?

twpayne · 2024-05-20T08:26:18Z


+// uniqTemplateFunc is the core implementation of the `uniq` template function.
+func uniqTemplateFunc(list []any) []any {
+	seen := make(map[any]struct{})


This will not work as expected. For example, see https://go.dev/play/p/qemCBLcOyPh. I think it can fail in other ways too.

bradenhilton · 2024-05-20T16:37:03Z

I think this highlights that we need more robust test data. I didn't notice most of these because I was only testing with lists, which are always []any (in addition to many incorrect assumptions).

It would help to test with some predefined []any/[]string/[]int etc., given that we want to maintain types where possible.

bradenhilton · 2024-05-20T19:07:12Z

Okay, I'm going to essentially start over in some new commits.

First up is (a hopefully better) compact...

bradenhilton · 2024-05-25T23:25:15Z

I might squash and merge this tomorrow (unless there are more mistakes) as I can't really come up with any more list function implementations that I'm happy with at the moment.

It could benefit from a quality pass for error message consistency etc. and also to revert the Go version change, as it's only necessary for the slices package, which I haven't used in compact or reverse.

I can make reverse work in-place if preferred.

I'm also happy to change

if v.Kind() != reflect.Slice {
    ...
}

to a switch statement that includes both reflect.Slice and reflect.Array, if preferred.

bradenhilton force-pushed the list branch from acb3312 to 6241cb0 Compare May 18, 2024 13:00

bradenhilton added 2 commits May 18, 2024 20:08

feat: add more list funcs

bf5e4c5

reverse

3fd6298

bradenhilton force-pushed the list branch from db09d3c to dfadab3 Compare May 18, 2024 19:12

terrible sort

b674fe9

bradenhilton force-pushed the list branch from dfadab3 to b674fe9 Compare May 18, 2024 19:14

bradenhilton commented May 18, 2024

View reviewed changes

bradenhilton added 3 commits May 19, 2024 13:57

ugly sort

dc69ef8

slightly less ugly sort

5d26c4e

uniq

0e91528

bradenhilton force-pushed the list branch from c3735ad to 7704cc7 Compare May 19, 2024 20:37

concat

ea3fac2

bradenhilton force-pushed the list branch from 7704cc7 to ea3fac2 Compare May 19, 2024 20:40

twpayne reviewed May 20, 2024

View reviewed changes

bradenhilton added 2 commits May 20, 2024 20:07

new compact

f97e568

new reverse

ccbc3a5

Conversation

bradenhilton commented May 18, 2024

Uh oh!

bradenhilton commented May 18, 2024

Uh oh!

bradenhilton commented May 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

twpayne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradenhilton May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradenhilton commented May 20, 2024

Uh oh!

bradenhilton commented May 20, 2024

Uh oh!

bradenhilton commented May 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

bradenhilton May 20, 2024 •

edited

Loading