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

Clean up storage.PutOptions #2596

Merged
merged 4 commits into from
Nov 15, 2023
Merged

Clean up storage.PutOptions #2596

merged 4 commits into from
Nov 15, 2023

Conversation

bufdev
Copy link
Member

@bufdev bufdev commented Nov 15, 2023

Use an interface so these can only be built by the storage package, clean up docs, clean up meaning of 0.

// This is a suggestion, implementations may choose to ignore this option.
func PutWithSuggestedChunkSize(suggestedChunkSize int) PutOption {
return func(putOptions *putOptions) {
putOptions.suggestedChunkSize = &suggestedChunkSize
Copy link
Member

Choose a reason for hiding this comment

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

Should we ignore negative values here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

}

func (p *putOptions) SuggestedDisableChunking() bool {
return p.suggestedChunkSize != nil && *p.suggestedChunkSize == 0
Copy link
Member

@pkwarren pkwarren Nov 15, 2023

Choose a reason for hiding this comment

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

If we don't prevent negative values we should change the conditional from == to <= here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@bufdev bufdev merged commit 8cbef4a into main Nov 15, 2023
7 checks passed
@bufdev bufdev deleted the put-options-cleanup branch November 15, 2023 23:01
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