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(cli): replace promptly with dialoguer #3669

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

paolobarbolini
Copy link
Contributor

Turns out #3665 didn't fully fix the build for loongarch64 and s390x because the promptly crate depends on an old version of nix. While looking more into it I found that promptly is unmaintained and is used very little (judging by the reverse dependencies on crates.io and number of downloads). I've decided to replace it with dialoguer, maintained by console-rs org from which other crates sqlx-cli is already depending on.

This also happens to reduce the number of indirect dependencies, which is nice.

@abonander
Copy link
Collaborator

Ironically, we previously replaced dialoguer with promptly to fix #1109

Did dialoguer address this?

@abonander
Copy link
Collaborator

Alternatively, since we don't really need anything super fancy, we could just roll our own prompts, couldn't we?

@abonander
Copy link
Collaborator

If it's a just a matter of handling Ctrl-C/SIGINT, we can do that through Tokio, but we'd need to run the dialoguer prompt in a blocking task so it doesn't block the runtime.

@paolobarbolini
Copy link
Contributor Author

Did dialoguer address this?

It still happens.

If I understand correctly, it's the opinion of the dependecy's developers that this should be handled by users of the crate

Disappointing 🙄

If it's a just a matter of handling Ctrl-C/SIGINT, we can do that through Tokio, but we'd need to run the dialoguer prompt in a blocking task so it doesn't block the runtime.

I may have a simpler idea but first I have to figure out how to restore the cursor.

@abonander
Copy link
Collaborator

I may have a simpler idea but first I have to figure out how to restore the cursor.

console-rs/dialoguer#22 (comment)

Copy link
Contributor Author

@paolobarbolini paolobarbolini left a comment

Choose a reason for hiding this comment

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

What I was thinking didn't really work. I've tried this other way but it too had problems 🙄

sqlx-cli/src/database.rs Outdated Show resolved Hide resolved
sqlx-cli/src/database.rs Outdated Show resolved Hide resolved
println!("{e}");
return false;
async fn ask_to_continue_drop(db_url: String) -> bool {
struct RestoreCursorGuard {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd be a good idea to comment on why this is necessary.


select! {
biased;
_ = ctrlc_fut => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should mention why we need to explicitly handle Ctrl-C.

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