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

feat(spinner): improve action #292

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

feat(spinner): improve action #292

wants to merge 11 commits into from

Conversation

caarlos0
Copy link
Member

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.

@caarlos0 caarlos0 added the enhancement New feature or request label Jun 24, 2024
@caarlos0 caarlos0 self-assigned this Jun 24, 2024
@caarlos0 caarlos0 requested a review from maaslalani as a code owner June 24, 2024 17:59
spinner/spinner.go Outdated Show resolved Hide resolved
spinner/examples/loading/main.go Outdated Show resolved Hide resolved
spinner/spinner.go Outdated Show resolved Hide resolved
spinner/spinner.go Outdated Show resolved Hide resolved
@caarlos0
Copy link
Member Author

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 Spinner2[Something, error] I would guess, maybe even worth making the second arg an error always.

@caarlos0
Copy link
Member Author

caarlos0 commented Jun 25, 2024

@maaslalani I reworked some stuff:

  • run the action as part as a tea.Cmd
  • improve handling when having only ctx, only action, both, or neither

@caarlos0
Copy link
Member Author

I think this might also fix #196

@caarlos0
Copy link
Member Author

one bad thing about this, is that it might be a breaking change 🤔

@ccoVeille
Copy link

Can't you support both signature with generics? 🤔

spinner/spinner.go Outdated Show resolved Hide resolved
spinner/spinner.go Outdated Show resolved Hide resolved
Copy link

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@caarlos0 caarlos0 changed the title feat(spinner): make action return an error feat(spinner): improve action Jul 25, 2024
@caarlos0
Copy link
Member Author

also added the feature from #172 here

err := spinner.New().
Context(ctx).
ActionErr(func(context.Context, io.Writer) error {
time.Sleep(time.Minute)

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

Suggested change
time.Sleep(time.Minute)
select {
case <-time.After(5 * time.Second):
case <-ctx.Done():
}

spinner/spinner.go Show resolved Hide resolved
@caarlos0 caarlos0 requested review from aymanbagabas and bashbunni and removed request for maaslalani August 9, 2024 22:55
return s
}

// ActionErr sets the action of the spinner.
Copy link
Member

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

@bashbunni
Copy link
Member

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?

@caarlos0
Copy link
Member Author

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

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

Successfully merging this pull request may close these issues.

3 participants