Skip to content

implement Transform,TransformSlice,InsertAllFunc(#4,#16,#23)#56

Merged
shoenig merged 3 commits intohashicorp:mainfrom
zonewave:feat_common
May 4, 2023
Merged

implement Transform,TransformSlice,InsertAllFunc(#4,#16,#23)#56
shoenig merged 3 commits intohashicorp:mainfrom
zonewave:feat_common

Conversation

@zonewave
Copy link
Copy Markdown
Contributor

@zonewave zonewave commented May 1, 2023

  1. I set a commonSet interface
type CommonSet[T any] interface {
	Slice() []T
	Insert(T) bool
	InsertSlice([]T) bool
	Size() int
	// ForEach  will call the callback function for each element in the set
	// If the callback returns false, the iteration will stop
	ForEach(call func(T) bool)
}
  1. add a IterCallback(call func(T) bool) method for set,hashset ,treeset.
  2. implement InsertSliceFunc, Transform, TransformSlice

@zonewave zonewave force-pushed the feat_common branch 2 times, most recently from 72c70e2 to d52bfcb Compare May 1, 2023 17:51
Copy link
Copy Markdown
Contributor

@shoenig shoenig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great @zonewave, I commented some suggestions / tweaks - let me know what you think

Comment thread common.go Outdated
Comment thread common.go
Comment thread common_test.go Outdated
Comment thread common.go Outdated
Comment thread common.go Outdated
Comment thread common.go Outdated
Comment on lines +21 to +27
// Transform transforms the set a into another set b
func Transform[T, E any](a CommonSet[T], b CommonSet[E], transform func(T) E) {
a.ForEach(func(item T) bool {
_ = b.Insert(transform(item))
return true
})
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to have two versions of this function? One that transforms into an existing set like this implementation, but also one that creates the new set? e.g.

TransformUnion[T, E any](a CommonSet[T], b CommonSet[E], f func(T) E)
Transform[T, E any](s CommonSet[T], f func(T) E) CommonSet[E]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm to rename transform as TransformUnion.
The other transform is not implemented for now, it may have to be implemented next time.

Comment thread common.go Outdated
Comment on lines +37 to +40
func Transform[T, E any](a Common[T], transform func(T) E, newSet func() Common[E]) Common[E] {
b := newSet()
TransformUnion(a, b, transform)
return b
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh I didn't think of the newSet problem. Sorry to keep you going back and forth @zonewave, but can you remove Transform for now. I want to think if there's a cleaner way we can do this, without having to pass in a function to construct a new set. There's probably some generics magic + type switching we can do here, but I need time to think about it.

Copy link
Copy Markdown
Contributor Author

@zonewave zonewave May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, I also don't think it looks elegant.I have removed the last submission

Copy link
Copy Markdown
Contributor

@shoenig shoenig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; thanks @zonewave

@shoenig shoenig merged commit 24d6066 into hashicorp:main May 4, 2023
@zonewave zonewave deleted the feat_common branch May 10, 2023 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants