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

Added bucket operations to supported functionality #27

Merged

Conversation

thepocp
Copy link
Contributor

@thepocp thepocp commented Feb 8, 2024

This PR introduces new functionalities to the client. The added methods allow for checking if a bucket exists, creating a new bucket, and removing an existing bucket in the client. These methods are bucketExists, makeBucket, and removeBucket respectively.

Copy link
Owner

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this contribution - looks great! I'll merge tomorrow and release a new version. Let me know what you think about the one line I commented on.

public async makeBucket(bucketName: string): Promise<void> {
await this.makeRequest({
method: "PUT",
bucketName: this.getBucketName({ bucketName }),
Copy link
Owner

Choose a reason for hiding this comment

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

Since we're calling this.getBucketName, we could specify that the bucketName: string parameter is optional, and if it's undefined it would fall back to this.defaultBucket. But I guess that even if we're not using that functionality here, this.getBucketName() is still serving a useful purpose of validating the bucket name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first, I thought about allowing the bucketName to be optional, but I ultimately concluded that emphasizing clarity by requiring it explicitly was the best approach

@bradenmacdonald bradenmacdonald merged commit b239dbb into bradenmacdonald:main Feb 9, 2024
1 check passed
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.

None yet

2 participants