-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat: clean command prompts for use case #35
Conversation
pkg/aspect/clean/clean.go
Outdated
// then ask | ||
// do you want to see this wizard again next time? | ||
// and if not, record in the cache file to inhibit next time | ||
skip := viper.GetBool(skipPromptKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a singleton really hard to mock for testing. I wonder if we should wrap this so we have better control.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pkg/aspect/clean/clean.go
Outdated
// do you want to see this wizard again next time? | ||
// and if not, record in the cache file to inhibit next time | ||
skip := viper.GetBool(skipPromptKey) | ||
interactive := isatty.IsTerminal(os.Stdout.Fd()) || isatty.IsCygwinTerminal(os.Stdout.Fd()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a field in the Clean
struct that gets populated when we construct the command. Again, makes it easier to test, since we should not access the real IO streams inside the pkg
tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah we should even have an assertion on the dep graph like "pkg should not import os" or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I never thought about doing something like this. We can try. Maybe a bit hard, but sounds correct.
pkg/aspect/clean/clean.go
Outdated
|
||
case 0: | ||
// Allow user to opt-out of our fancy "clean" command and just behave like bazel | ||
fmt.Println("You can skip this prompt to make 'aspect clean' behave the same as 'bazel clean'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the print statements below should be in the format of:
fmt.Fprintf(c.Streams.Stdout, "foo\n")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, and added some assertions on the stdout
pkg/aspect/clean/clean.go
Outdated
}, | ||
} | ||
|
||
i, _, err := choose.Run() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea for testing these proptui calls is to make the promptui.Select
s an interface. As simple as:
type PromptSelectRunner interface {
Run() (int, string, error)
}
The default constructor can then receive the prompui.Select
struct initialization and on tests we just create mocks out of the interface.
Since we are passing the IO streams as interfaces, these can also receive e.g. a bytes.Buffer
instead of os.Stdout
so we can capture the output and make assertions on the selections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that works pretty well
ae9ec31
to
cffe056
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Much better!
No description provided.