Skip to content

Change uiutil from survey to pterm#4853

Draft
afbjorklund wants to merge 2 commits intolima-vm:masterfrom
afbjorklund:pterm
Draft

Change uiutil from survey to pterm#4853
afbjorklund wants to merge 2 commits intolima-vm:masterfrom
afbjorklund:pterm

Conversation

@afbjorklund
Copy link
Copy Markdown
Member

Closes #1881

image

Signed-off-by: Anders F Björklund <anders.f.bjorklund@gmail.com>
Signed-off-by: Anders F Björklund <anders.f.bjorklund@gmail.com>
Copy link
Copy Markdown

@liketosweep liketosweep left a comment

Choose a reason for hiding this comment

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

Really glad to see this moving forward survey being archived was a pain point worth addressing.

A few things I noticed while looking through the diff that might be worth considering:

  • The two failing QEMU integration tests feel related to go-isatty being removed from the imports. survey used it for explicit TTY detection pterm may not handle non-TTY stdin (containers, piped input, SSH without a PTY) as gracefully, which could explain the failures. Might be worth a quick TTY check before invoking any interactive prompt.
  • pterm brings in quite a few more transitive dependencies compared to survey. Not necessarily a problem, just something to be mindful of from a binary size and supply chain perspective.

On the suggestion side it might be nice to expose a defaultValue bool parameter on ConfirmInput down the line. survey had this and it's handy for callers who want a sensible fallback in automated or scripted workflows without blocking on user input.

None of this is a criticism the public API is preserved cleanly and the interrupt handling via OnInterruptFunc is genuinely an improvement over what was there before. Just flagging in case any of it is useful!

@afbjorklund
Copy link
Copy Markdown
Member Author

Sorry, but that does not seem to be matching the code?

The github.com/mattn/go-isatty mod is unchanged...
Only an older version was removed from the go.sum

There is no significant change in the binary size:
(the number of dependencies are comparable)
33M _output/bin/limactl # before
33M _output/bin/limactl # after

There is no method called ConfirmInput, only Confirm
And it does have a defaultParam parameter already.

The interrupt function is a bit of a workaround to return
an error, but it is potentially more capable (not used here).

@liketosweep
Copy link
Copy Markdown

Thanks for the corrections, @afbjorklund. I definitely misread the go.sum changes as an import removal and missed the defaultParam already present in Confirm. Appreciate you taking the time to verify the binary size as well it’s great to know the switch is so lean. Thanks for the clarification!

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.

User interface library "survey" has been archived

2 participants