-
Notifications
You must be signed in to change notification settings - Fork 132
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
feat(spinner): improve action #292
base: main
Are you sure you want to change the base?
Conversation
another idea would be to have a type Spinner[T any] struct {
action func () T
// .. and maybe a type Spinner2[T any, Q any] struct {
action func() (T, Q)
} the later being most commonly used as |
@maaslalani I reworked some stuff:
|
Signed-off-by: Carlos Alexandro Becker <[email protected]>
I think this might also fix #196 |
one bad thing about this, is that it might be a breaking change 🤔 |
Can't you support both signature with generics? 🤔 |
Signed-off-by: Carlos Alexandro Becker <[email protected]>
Signed-off-by: Carlos Alexandro Becker <[email protected]>
Signed-off-by: Carlos Alexandro Becker <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
also added the feature from #172 here |
err := spinner.New(). | ||
Context(ctx). | ||
ActionErr(func(context.Context, io.Writer) error { | ||
time.Sleep(time.Minute) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even it's an example, I won't use a time. Minute here
You can use a 5s delay it would be enough.
I know this timeout won't be reached but three is a problem with a time.Sleep that will block everything. It's somehow OK for an example. But as it's an example, it should be written in a production-ready code
So here is what I'm suggesting
time.Sleep(time.Minute) | |
select { | |
case <-time.After(5 * time.Second): | |
case <-ctx.Done(): | |
} |
return s | ||
} | ||
|
||
// ActionErr sets the action of the spinner. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should specify what the difference is between Action
and ActionErr
I think I'm missing something obvious here - what's wrong with how we're currently handling actions and how does this change improve that? |
if i recall correctly, if you passed both a context and a function it didn't work properly, that's fixed here |
I think it would be great to allow the spinner action to return an error, so it simplifies error handling in some cases.
this pr is wip to see what you think, if you think its a good idea, I'll work more on it.