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

Proposal: Exported default variables should be constants #40

Closed
Noroth opened this issue Jan 15, 2021 · 2 comments · Fixed by #44
Closed

Proposal: Exported default variables should be constants #40

Noroth opened this issue Jan 15, 2021 · 2 comments · Fixed by #44

Comments

@Noroth
Copy link

Noroth commented Jan 15, 2021

When defining default values, it normally makes no sense to make them mutable. Global default values should either be defined as constants, or be well documented, if there are use cases to globally change the default behavior.

Maybe adding a function in config.go, that creates a new default instance and removing the const block in rety.go would be a better approach here?

func NewWithDefaults() *Config {
	return &Config{
		attempts:      uint(10),
		delay:         100 * time.Millisecond,
		maxJitter:     100 * time.Millisecond,
		onRetry:       func(n uint, err error) {},
		retryIf:       IsRecoverable,
		delayType:     CombineDelay(BackOffDelay, RandomDelay),
		lastErrorOnly: false,
		context:       context.Background(),
	}
}
@JaSei
Copy link
Collaborator

JaSei commented May 12, 2021

Hi, @Noroth, I agree exported variables aren't nice. I don't know why I choose this way. Maybe because to the const isn't possible to set function? Or I don't know, and it doesn't matter. I like your idea of NewWithDefaults, and it looks for me like goish way how to do it. I will do it, or PR is really welcome.

Thanks

@Noroth
Copy link
Author

Noroth commented May 12, 2021

Hi @JaSei, I can create a PR later and link it to this issue.

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 a pull request may close this issue.

2 participants