Skip to content

feat(core): implement delete with recursive #5762

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

feat(core): implement delete with recursive #5762

wants to merge 2 commits into from

Conversation

meteorgan
Copy link
Contributor

@meteorgan meteorgan commented Mar 13, 2025 •
edited
Loading

Which issue does this PR close?

Part of #3922.

Rationale for this change

implement recursive deletion

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the releases-note/feat The PR implements a new feature or has a title that begins with "feat" label Mar 13, 2025
meteorgan
meteorgan commented Mar 13, 2025
View reviewed changes
@meteorgan meteorgan force-pushed the delete_with_recursive branch from a54c432 to 9a9958a Compare March 13, 2025 15:50
Xuanwo
Xuanwo reviewed Mar 13, 2025
View reviewed changes
@meteorgan meteorgan force-pushed the delete_with_recursive branch 2 times, most recently from 40a2aa7 to 3ad7ce5 Compare March 19, 2025 13:29
@meteorgan meteorgan changed the title feat(core): support delete with recursive and versions feat(core): implement delete with recursive Mar 19, 2025
@meteorgan
Copy link
Contributor Author

I'm considering another approach. Why don't we implement this functionality in Deleter ? It would make it more explicit. what do you think ? @Xuanwo

@Xuanwo
Copy link
Member

Xuanwo commented Mar 19, 2025 •
edited
Loading

I'm considering another approach. Why don't we implement this functionality in Deleter ? It would make it more explicit. what do you think ? @Xuanwo

Would you like to make it more explicit? What's your current plan?

@meteorgan
Copy link
Contributor Author

meteorgan commented Mar 19, 2025 •
edited
Loading

Would you like to make it more explicit? What's your current plan?

I've basically implemented recursive deletion in CompleteLayer, but the code looks somewhat similar to Deleter and BatchDeleter, you can take a look. I'm considering whether implementing this logic in Deleter would be a better choice.

@meteorgan
Copy link
Contributor Author

Besides, The logic in CompleteLayer is pretty hidden, Moving it to Deleter would make it more obvious and straightforward.

@meteorgan
Copy link
Contributor Author

Or, we could implement RecursiveDeleter based on Deleter

@meteorgan meteorgan force-pushed the delete_with_recursive branch from ee717fc to 701b179 Compare April 7, 2025 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Xuanwo Xuanwo Xuanwo left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

releases-note/feat The PR implements a new feature or has a title that begins with "feat"

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@meteorgan @Xuanwo