Adds prompt to overwrite when rerunning scaffold init #61
Conversation
|
|
jcsackett
force-pushed
the
feat/prompt-when-scaffold-yaml-exists
branch
2 times, most recently
from
1e9fc34
to
ba4c401
Compare
cmd/scaffold.go
Outdated
| return nil | ||
| }, | ||
| } // Not using IsConfirm; it seems to have rendering issues in a few terminal cases. | ||
| if result, err := prompt.Run(); err == nil && result == "y" { |
There was a problem hiding this comment.
Should we propagate the error if err != nil ?
There was a problem hiding this comment.
We can; I had thought that in the event where Prompt.Run failed something weird enough happened that we should just do nothing (e.g. return nil) but that does obfuscate that something has indeed gone wrong.
I'll update to return the error; when expanding the use of promptui maybe I can put together something better to handle Run errors than just returning it to the the user.
jcsackett
force-pushed
the
feat/prompt-when-scaffold-yaml-exists
branch
from
8f5a32b
to
2c11be5
Compare
| vervetApp, ok := ctx.Context.Value(vervetKey).(*VervetApp) | ||
| if !ok { | ||
| return errors.New("could not retrieve vervet app from context") | ||
| } |
There was a problem hiding this comment.
A func AppFromContext(ctx context.Context) *VervetApp helper might be nice for this. If not in this PR, please leave a TODO. vervet version new also needs a prompting capability, it can be refactored when we get to that one.
There was a problem hiding this comment.
I'm strongly in agreement with this idea, thanks! I've left the TODO for the follow up. A contextWithApp helper might be cleaner too where it's set up in cmd/cmd.go
cmd/scaffold.go
Outdated
| @@ -34,7 +35,27 @@ func ScaffoldInit(ctx *cli.Context) error { | |||
| } | |||
| err = sc.Organize() | |||
| if err != nil { | |||
There was a problem hiding this comment.
I'm wondering if os.IsExist would be a more precise test for whether we need to prompt. It'd be nice if we only prompt on a failure to overwrite, and not other things (like no file permissions) to avoid confusing behavior.
There was a problem hiding this comment.
os.IsExist can't actually be used here; the error being returned doesn't unwrap to one that matches for that.
Instead I've added a custom error--ErrAlreadyExists--in internal/scaffold/scaffold.go, and added logic to check if the files already exist as we're copying them over and return that, which is then used as the key for running the prompt.
Currently running `scaffold init` a second time results in an error message being propogated to the user as the cli output. This is harmless, as the error regards the files being created already existing, but not a great UX. This adds promptui to run an interactive prompt for overwriting the files. If the user selects to overwite, the command behaves as though it were run with the `-force` option. If the user doesn't, it ends the command but doesn't return an error. Other related changes introduced by this change: * The cli app is wrapped in a new VervetApp struct, allowing config options specific to vervet but not to the CLI interface to be set-- one of these options is a prompting interface. * The vervet app is added to the context passed into the cli app, so that vervet specific info can be used in the commands called by the cli app. Fixes #56
jcsackett
force-pushed
the
feat/prompt-when-scaffold-yaml-exists
branch
from
0589b6f
to
8d20aec
Compare
Currently running
scaffold inita second time results in an errormessage being propogated to the user as the cli output. This is
harmless, as the error regards the files being created already existing,
but not a great UX.
This adds promptui to run an interactive prompt for overwriting the
files. If the user selects to overwite, the command behaves as though it
were run with the
-forceoption. If the user doesn't, it ends thecommand but doesn't return an error.
Other related changes introduced by this change:
options specific to vervet but not to the CLI interface to be set--
one of these options is a prompting interface.
that vervet specific info can be used in the commands called by the
cli app.
Fixes #56