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

refactor: removeObject #1161

Merged
merged 21 commits into from
Jun 28, 2023
Merged

refactor: removeObject #1161

merged 21 commits into from
Jun 28, 2023

Conversation

trim21
Copy link
Contributor

@trim21 trim21 commented May 24, 2023

This PR refactors Client.removeObject to TypeScript.

What's maintainers' option on this, is this an acceptable change?

ideally, refactered method should use async function and we can wrapped them with callbackify to handle potential latest callback function argument.

this also make it easier to add arguments, no need to do this:

    // backward compatibility
    if (isFunction(removeOpts)) {
      cb = removeOpts
      removeOpts = {}
    }

But currently, arguments are checked synchronously.

for example: callback can't catch the error throwed by method.

client.removeObject(null, '...', (err,result)=>{})

they need to catch null bucket name error like this:

try{
  client.removeObject(null, '...', (err,result)=>{})
} catch(e) {
  ...
}

after this PR, client.removeObject(null, '...', (err,result)=>{}) will not throw, but always return error to callback or promise reject.

also, mark callback style API as @deprecated

@trim21 trim21 marked this pull request as ready for review May 24, 2023 21:49
Copy link
Contributor

@aldy505 aldy505 left a comment

Choose a reason for hiding this comment

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

LGTM

@prakashsvmx
Copy link
Member

I too have evaluated alternatives and the suggestion. yes. We can go ahead with this for the flexibility it offers. 👍 @trim21

@trim21
Copy link
Contributor Author

trim21 commented Jun 13, 2023

I too have evaluated alternatives and the suggestion. yes. We can go ahead with this for the flexibility it offers. 👍 @trim21

This make thing much easier

@trim21
Copy link
Contributor Author

trim21 commented Jun 24, 2023

@prakashsvmx i mark callback style api as deprecated, hope this is not a problem

@trim21
Copy link
Contributor Author

trim21 commented Jun 25, 2023

@kaankabalak

prakashsvmx
prakashsvmx previously approved these changes Jun 26, 2023
Copy link
Member

@prakashsvmx prakashsvmx left a comment

Choose a reason for hiding this comment

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

Tested.

- forceDelete
- versionId
- governanceBypass  w/o versionId

Changes look good to me.

docs/API.md Outdated Show resolved Hide resolved
docs/API.md Outdated Show resolved Hide resolved
docs/API.md Outdated Show resolved Hide resolved
Co-authored-by: Kaan Kabalak <[email protected]>
trim21 and others added 2 commits June 27, 2023 18:07
Copy link
Contributor

@kaankabalak kaankabalak left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants