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

lib: improve wait.For returns. #1403

Merged
merged 1 commit into from
May 14, 2021
Merged

Conversation

ldez
Copy link
Member

@ldez ldez commented May 14, 2021

Fixes #1401

@ldez ldez added this to the v4.4 milestone May 14, 2021
@ldez ldez requested a review from dmke May 14, 2021 12:23
@quenbyako
Copy link
Contributor

quenbyako commented May 14, 2021

@ldez i though about something different which is using context with timeout)

something like

func For(ctx context.Context, repeatInterval time.Duration, f func() (bool, error)) error {
    ...
}

also, i don't think that it's a good idea to return last error: if f returns error, it must be throwed explicitly, cause you can lost more important errors in the middle of updating process.

@ldez
Copy link
Member Author

ldez commented May 14, 2021

For now, I don't want to change the signature of the function.

also, i don't think that it's a good idea to return last error: if f returns error, it must be throwed explicitly, cause you can lost more important errors in the middle of updating process.

this is the goal of this function to not throw an error immediately and to only return the last error.

@ldez ldez merged commit c53c3d0 into go-acme:master May 14, 2021
@ldez ldez deleted the fix/wait-for-timeout branch May 14, 2021 15:37
@quenbyako
Copy link
Contributor

@ldez return the last error

Ehhh, okay, it just looks pretty weird for me, but okay, you're a member and i'll not argue for my subjective opinion, so alright)

Also, thanks a lot for so fast fix and update! I really appreciate that 🤗

@dmke
Copy link
Member

dmke commented May 14, 2021

it just looks pretty weird for me

Naming is hard. A better name for For() would have been RetryUntil().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

wait.For() can return ambigous errors
3 participants