rueidis icon indicating copy to clipboard operation
rueidis copied to clipboard

Support Go 1.23 range over func in internal/cmds

Open tamayika opened this issue 1 year ago • 4 comments

I know Builder respects Redis command argument order, but this is a little annoying in some situation.

For example, please consider maxlen is set only when limit > 0 for XADD. Currently I have to write like blow.

func xadd(ctx context.Context, conn rueidis.Client, key string, limit int, attributes map[string]string) error {
	xadd := conn.B().Xadd().Key(key)
	var completed rueidis.Completed
	if limit > 0 {
		fieldValue := xadd.Maxlen().Almost().Threshold(strconv.Itoa(limit)).Id("*").FieldValue()
		for k, v := range attributes {
			fieldValue = fieldValue.FieldValue(k, v)
		}
		completed = fieldValue.Build()
	} else {
		fieldValue := xadd.Id("*").FieldValue()
		for k, v := range attributes {
			fieldValue = fieldValue.FieldValue(k, v)
		}
		completed = fieldValue.Build()
	}
	return conn.Do(ctx, completed).Error()
}

If XaddFieldValue is exported, I can write like below.

func xadd(ctx context.Context, conn rueidis.Client, key string, limit int, attributes map[string]string) error {
	xadd := conn.B().Xadd().Key(key)
	var fieldValue rueidis.XaddFieldValue
	if limit > 0 {
		fieldValue = xadd.Maxlen().Almost().Threshold(strconv.Itoa(limit)).Id("*").FieldValue()
	} else {
		fieldValue = xadd.Id("*").FieldValue()
	}
	for k, v := range attributes {
		fieldValue = fieldValue.FieldValue(k, v)
	}
	return conn.Do(ctx, fieldValue.Build()).Error()
}

tamayika avatar Aug 29 '23 10:08 tamayika

Hi @tamayika,

I understand that it may be annoying for some people. However, I don't think making these intermediate builder public is a worthwhile solution. Because:

  1. It is weird to only export selected intermediate builders. We have to export them all if we are going this way.
  2. But they are too many and will make rueidis.XXX auto-completion be messy.
  3. They will introduce false usages easily, for example:
var fieldValue rueidis.XaddFieldValue
fieldValue.Build()

IMHO, the following code style, without branching and merging while constructing a command, is actually preferable:

func xadd(ctx context.Context, conn rueidis.Client, key string, limit int, attributes map[string]string) error {
	if limit > 0 {
		fieldValue := conn.B().Xadd().Key(key).Maxlen().Almost().Threshold(strconv.Itoa(limit)).Id("*").FieldValue()
		for k, v := range attributes {
			fieldValue = fieldValue.FieldValue(k, v)
		}
		return conn.Do(ctx, fieldValue.Build()).Error()
	} else {
		fieldValue := conn.B().Xadd().Key(key).Id("*").FieldValue()
		for k, v := range attributes {
			fieldValue = fieldValue.FieldValue(k, v)
		}
		return conn.Do(ctx, fieldValue.Build()).Error()
	}
}

If you are concerned about the duplicated for k, v := range attributes, I think we can probably have a FieldValueWith() helper, so that:

func xadd(ctx context.Context, conn rueidis.Client, key string, limit int, attributes map[string]string) error {
	if limit > 0 {
		return conn.Do(ctx, conn.B().Xadd().Key(key).Maxlen().Almost().Threshold(strconv.Itoa(limit)).Id("*").FieldValueWith(attributes).Build()).Error()
	} else {
		return conn.Do(ctx, conn.B().Xadd().Key(key).Id("*").FieldValueWith(attributes).Build()).Error()
	}
}

rueian avatar Aug 29 '23 15:08 rueian

But they are too many and will make rueidis.XXX auto-completion be messy.

How about adding sub package like cmds to export all commands? I think it does not make rueidis package dirty.

They will introduce false usages easily, for example:

Sorry, I don't understand what you mean. This happens without exported type.

conn.B().Xadd().Key(key).Id("*").FieldValue().Build()

If you are concerned about the duplicated for k, v := range attributes, I think we can probably have a FieldValueWith() helper, so that:

It is effective only when to set map[string]string, there are other examples like map[string]int or well typed struct. Maybe we should wait range over func to avoid temporary map allocation for above types. If this landed, we can write like blow.

// internal/cmds
package cmds

func (c XaddFieldValue) FieldValueIter(iter iter.Seq2[string, string]) XaddFieldValueIter {
	for key, value := range iter {
		c.cs.s = append(c.cs.s, key, value)
	}
	return (XaddFieldValueIter)(c)
}

type XaddFieldValueIter Completed

func (c XaddFieldValueIter) Build() Completed {
	c.cs.Build()
	return Completed(c)
}

map[string]string

func xadd(ctx context.Context, conn rueidis.Client, key string, limit int, attributes map[string]string) error {
	xadd := conn.B().Xadd().Key(key)
	iter := func(yield func(string, string) bool) bool {
		for key, value := range attributes {
			if !yield(key, value) {
				return false
			}
		}
		return true
	}
	var completed rueidis.Completed
	if limit > 0 {
		completed = xadd.Maxlen().Almost().Threshold(strconv.Itoa(limit)).Id("*").FieldValueIter(iter).Build()
	} else {
		completed = xadd.Id("*").FieldValueIter(iter).Build()
	}
	return conn.Do(ctx, completed).Error()
}

map[string]int

func xadd(ctx context.Context, conn rueidis.Client, key string, limit int, attributes map[string]int) error {
	xadd := conn.B().Xadd().Key(key)
	iter := func(yield func(string, string) bool) bool {
		for key, value := range attributes {
			if !yield(key, strconv.Itoa(value)) {
				return false
			}
		}
		return true
	}
	var completed rueidis.Completed
	if limit > 0 {
		completed = xadd.Maxlen().Almost().Threshold(strconv.Itoa(limit)).Id("*").FieldValueIter(iter).Build()
	} else {
		completed = xadd.Id("*").FieldValueIter(iter).Build()
	}
	return conn.Do(ctx, completed).Error()
}

struct

func xadd(ctx context.Context, conn rueidis.Client, key string, limit int, person Person) error {
	xadd := conn.B().Xadd().Key(key)
	iter := func(yield func(string, string) bool) bool {
		if !yield("name", person.Name) {
			return false
		}
		if !yield("age", strconv.Itoa(person.Age)) {
			return false
		}
		return true
	}
	var completed rueidis.Completed
	if limit > 0 {
		completed = xadd.Maxlen().Almost().Threshold(strconv.Itoa(limit)).Id("*").FieldValueIter(iter).Build()
	} else {
		completed = xadd.Id("*").FieldValueIter(iter).Build()
	}
	return conn.Do(ctx, completed).Error()
}

tamayika avatar Aug 30 '23 00:08 tamayika