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

fix: deprecated promptui library #399

Closed
wants to merge 2 commits into from

Conversation

codinja1188
Copy link
Contributor

What fixes the PR?

#331

if err != nil {
if !force {
fmt.Printf("Are you sure you want to delete device %s (y/N): ", organizationID)
userInput := prompt.Input(">", func(d prompt.Document) []prompt.Suggest {
Copy link
Member

@displague displague Nov 30, 2023

Choose a reason for hiding this comment

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

In this PR, most of the prompts are not using suggestions. If we choose not to use the suggestion feature, we are better off using a native fmt.Scanln and avoiding any dependency.

On the other hand, prompt suggestions (shown here and implemented (inconsistently) for port convert below) could offer a better interface for our metal init steps and could be used when a user is present to prompt (with suggestions) when a required value has been omitted (we can detect a terminal or stdin).

Copy link
Member

@displague displague Nov 30, 2023

Choose a reason for hiding this comment

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

In other words, let's go all in and introduce this to the metal init prompts and use yes/no suggestions for all confirmation prompts, or lets just use fmt.Sscanln and worry about more featureful UIs later (I thought we had an issue for that, suggesting TUI toolkits, but I don't see one).

This c-bata/go-prompt package could work for our input needs, but we may want to opt for something more comprehensive for our output needs. We don't offer field selection options in our table responses and we don't offer watch views. We could move in that direction with a package like https://github.com/rivo/tview or by adopting whatever mechanisms kubectl is using to support it's set feature set of tsv, jq, go template, json, yaml, and other output methods.

Copy link
Member

@displague displague Nov 30, 2023

Choose a reason for hiding this comment

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

I'd want to avoid introducing a fancy library (picker, autocomplete, dropdown, etc) to replace it with an aesthetically different one soon after. Going from our current prompt library to fmt.Scanln would avoid that with no user-facing change in experience.

Copy link
Contributor Author

@codinja1188 codinja1188 Nov 30, 2023

Choose a reason for hiding this comment

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

I will check the feasibility to adopt tview

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if !force {
		reader := bufio.NewReader(os.Stdin)
		fmt.Printf("Are you sure you want to delete device %s? (yes/no): ", deviceID)
		confirmation, err := reader.ReadString('\n')
		if err != nil {
			return fmt.Errorf("error reading confirmation: %w", err)
		}
		if strings.TrimSpace(strings.ToLower(confirmation)) != "yes" {
			fmt.Println("Device deletion cancelled.")
			return nil
		}
	}

Copy link
Contributor Author

@codinja1188 codinja1188 Dec 12, 2023

Choose a reason for hiding this comment

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

@displague @ctreatma @cprivitere

plz confirm the above approach

@displague
Copy link
Member

The initial concern of this PR has been addressed by #407

We discussed treating this PR as an experiment towards the introduction of some future TUI. We don't have an issue open for that at present and we aren't ready to consider alternatives at the moment.

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