Uh oh!
There was an error while loading. Please reload this page.
refactor(experiment): separate iostreams prompts from survey implementations - #404
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #404 +/- ##
==========================================
+ Coverage 67.89% 68.16% +0.27%
==========================================
Files 217 218 +1 Lines 18015 18020 +5 ==========================================
+ Hits 12231 12284 +53 + Misses 4632 4583 -49 - Partials 1152 1153 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zimeg
left a comment
There was a problem hiding this comment.
🔏 A few more notes on changes more!
🍀 I realize the reason to separate prompt interfaces from implementations wasn't noted in the description, but these changes hope to keep interfaces stable as the charm experiment reaches conclusion.
| }, nil) | ||
| cm.IO.On("InputPrompt", mock.Anything, "Enter an expression", iostreams.InputPromptConfig{ | ||
| Required: false, | ||
| }).Return("") |
There was a problem hiding this comment.
🧪 note: Lots of changes add the expected error value to mock prompts!
| if cfg.PageSize > 0 { | ||
| field.Height(cfg.PageSize + 2) | ||
| } |
| // ConfirmPrompt prompts the user for a "yes" or "no" (true or false) value for | ||
| // the message | ||
| func (io *IOStreams) ConfirmPrompt(ctx context.Context, message string, defaultValue bool) (bool, error) { | ||
| if io.config.WithExperimentOn(experiment.Charm) { | ||
| return charmConfirmPrompt(io, ctx, message, defaultValue) | ||
| } | ||
| return surveyConfirmPrompt(io, ctx, message, defaultValue) | ||
| } | ||
| // InputPrompt prompts the user for a string value for the message, which can | ||
| // optionally be made required | ||
| func (io *IOStreams) InputPrompt(ctx context.Context, message string, cfg InputPromptConfig) (string, error) { | ||
| if io.config.WithExperimentOn(experiment.Charm) { | ||
| return charmInputPrompt(io, ctx, message, cfg) | ||
| } | ||
| return surveyInputPrompt(io, ctx, message, cfg) | ||
| } | ||
| // MultiSelectPrompt prompts the user to select multiple values in a list and | ||
| // returns the selected values | ||
| func (io *IOStreams) MultiSelectPrompt(ctx context.Context, message string, options []string) ([]string, error) { | ||
| if io.config.WithExperimentOn(experiment.Charm) { | ||
| return charmMultiSelectPrompt(io, ctx, message, options) | ||
| } | ||
| return surveyMultiSelectPrompt(io, ctx, message, options) | ||
| } |
There was a problem hiding this comment.
👾 note: IIRC callsites often guard against flag inputs, but we might follow up with changes here to accept these in configurations so these prompts guard against scripting fallthrough!
🐌 ramble: I'm curious about the default behavior for confirm prompts too when interactive prompts aren't available since we might use the default choice instead of requiring flag alternatives to perhaps revisit the changes of #87?
There was a problem hiding this comment.
I agree, in non-TTY it should use the default choice and we should offer a flag for each confirm to override the choice.
There was a problem hiding this comment.
Thanks for a check here! Let's follow up with defaults and also options as changes to prompts become stable. We're not regressing with current behavior, but it's a known gap now!
…ams-prompts # Conflicts: # internal/iostreams/survey.go
Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
mwbrooks
left a comment
There was a problem hiding this comment.
✅ Looks more organized and thanks for improving the iostreams test coverage!
🧪 I've done a little local testing and I don't see any difference ✨
💬 I feel like a broken record, but I'll just ask that we try to stay focused on the survey-to-huh migration. I think this PR is helping us land in a more organized place with huh. But, I find it difficult to feel 100% confident in PRs that make large changes to our UI logic when our test coverage is still on the lower side.
| // ConfirmPrompt prompts the user for a "yes" or "no" (true or false) value for | ||
| // the message | ||
| func (io *IOStreams) ConfirmPrompt(ctx context.Context, message string, defaultValue bool) (bool, error) { | ||
| if io.config.WithExperimentOn(experiment.Charm) { | ||
| return charmConfirmPrompt(io, ctx, message, defaultValue) | ||
| } | ||
| return surveyConfirmPrompt(io, ctx, message, defaultValue) | ||
| } | ||
| // InputPrompt prompts the user for a string value for the message, which can | ||
| // optionally be made required | ||
| func (io *IOStreams) InputPrompt(ctx context.Context, message string, cfg InputPromptConfig) (string, error) { | ||
| if io.config.WithExperimentOn(experiment.Charm) { | ||
| return charmInputPrompt(io, ctx, message, cfg) | ||
| } | ||
| return surveyInputPrompt(io, ctx, message, cfg) | ||
| } | ||
| // MultiSelectPrompt prompts the user to select multiple values in a list and | ||
| // returns the selected values | ||
| func (io *IOStreams) MultiSelectPrompt(ctx context.Context, message string, options []string) ([]string, error) { | ||
| if io.config.WithExperimentOn(experiment.Charm) { | ||
| return charmMultiSelectPrompt(io, ctx, message, options) | ||
| } | ||
| return surveyMultiSelectPrompt(io, ctx, message, options) | ||
| } |
There was a problem hiding this comment.
I agree, in non-TTY it should use the default choice and we should offer a flag for each confirm to override the choice.
zimeg
commented
Mar 17, 2026
@mwbrooks Thank you for taking space to review these changesets 📚 🔍 ✨ I had gotten carried toward bringing the |
Uh oh!
There was an error while loading. Please reload this page.
Changelog
Summary
This PR separates
iostreamsprompts fromsurveyimplementations for improvements to test cases and planned changes toward standard flag handling.Reviewers
No change to logic with this change I hope!
Requirements