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

Export some commands in internal/cmds #355

Open
tamayika opened this issue Aug 29, 2023 · 2 comments
Open

Export some commands in internal/cmds #355

tamayika opened this issue Aug 29, 2023 · 2 comments
Labels
enhancement New feature or request

Comments

@tamayika
Copy link
Contributor

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()
}
@rueian
Copy link
Collaborator

rueian commented Aug 29, 2023

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()
	}
}

@tamayika
Copy link
Contributor Author

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()
}

@rueian rueian added the enhancement New feature or request label Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants