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

✨ Rename --force in delete to --confirm/--no-confirm #61

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Zethson
Copy link
Member

@Zethson Zethson commented Aug 14, 2024

See title.

@Zethson Zethson marked this pull request as ready for review August 14, 2024 09:08
lamin_cli/__main__.py Outdated Show resolved Hide resolved
@falexwolf
Copy link
Member

It's been so far a technical internal parameter.

I see that it'd be sometimes useful for CI runs, but I was worried about people accidentally using it.

If we think the danger is low, we should only briefly ponder whether this is an ideal name.

require-empty currently only checks whether all managed storage locations are empty, it ignores the database itself.

@Zethson
Copy link
Member Author

Zethson commented Aug 15, 2024

This is actually behavior that I would have expected under a --force parameter. However, the --force parameter currently skips the confirmation which isn't what I would have expected it to do. Having a --confirm/--no-confirm parameter instead of force and force behaving like require-empty sounds more intuitive to me.

@falexwolf
Copy link
Member

--confirm/--no-confirm to replace the current --force sounds awesome! That's a much better naming choice.

Having an "actual" --force that enables people to leave data unusably dangling with cryptic ids in cryptic storage locations still sounds like something I'd rather not expose to a user. It's rather small nuisance to manually delete a bunch of files but it's an absolute catastrophe if anyone ever accidentally adds --force, e.g., because they confuse it with --confirm.

@Zethson Zethson changed the title ✨ Add require-empty flag for deletion ✨ Rename --force in delete to --confirm/--no-confirm Aug 16, 2024
@Zethson
Copy link
Member Author

Zethson commented Aug 16, 2024

@falexwolf all right.
I changed this PR to now rename --force to --confirm/--no-confirm instead of it's earlier purpose.

@falexwolf
Copy link
Member

This is great! We'll just need to either update all notebooks and scripts everywhere to reflect it or this change should be made in a backward-compatible way together with renaming the param within lamindb-setup.

I suggest to make it backward compatible so that we have more time with the renaming (it'd be quite a bit of work).

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