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

fix: time.NewTicker method panics if argument is less than 0 #7

Merged
merged 3 commits into from
Jul 27, 2023

Conversation

lll-lll-lll-lll
Copy link
Contributor

What's Problem

The current implementation in the Client.Run method passes zero to the interval argument of the Wait method

prediction, err = r.Wait(ctx, *prediction, 0, 0)

This cause panic when initializing time.NewTicker.

ticker := time.NewTicker(interval)

Because time.NewTicker panics when its argument is a value less than or equal to zero.

func NewTicker(d Duration) *Ticker {
	if d <= 0 {
		panic(errors.New("non-positive interval for NewTicker"))
	}

tim.NewTicker()

What I did

  • modify comment on the interval arg
  • If interval arg is less than 0, returns error
  • Changed the arg of the Client.Wait method in the Client.Run method to 1.

@lll-lll-lll-lll lll-lll-lll-lll changed the title fix: time.NewTicker method panics if argument is "<= 0" fix: time.NewTicker method panics if argument is less than 0 Jul 25, 2023
run.go Outdated Show resolved Hide resolved
Copy link
Contributor

@mattt mattt left a comment

Choose a reason for hiding this comment

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

Thanks so much for your contribution, @lll-lll-lll-lll! I pushed a couple changes to update the docs and use 1 * time.Second as the default polling interval.

@mattt mattt merged commit b3b3e02 into replicate:main Jul 27, 2023
@mattt
Copy link
Contributor

mattt commented Jul 27, 2023

@lll-lll-lll-lll Thanks again for your help. This change is now available in v0.4.1.

@lll-lll-lll-lll
Copy link
Contributor Author

@mattt thank you for your review and update my PR

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