Skip to content

timetrace delete commands: Default to no when asking for confirmation#147

Merged
dominikbraun merged 1 commit intodominikbraun:mainfrom
jwnpoh:delete-command
Jul 2, 2021
Merged

timetrace delete commands: Default to no when asking for confirmation#147
dominikbraun merged 1 commit intodominikbraun:mainfrom
jwnpoh:delete-command

Conversation

@jwnpoh
Copy link
Copy Markdown
Contributor

@jwnpoh jwnpoh commented Jun 29, 2021

This PR addresses #110.

  • askForConfirmation() now defaults to no unless the input is specifically "y" or "Y".
  • added askForConfirmation() to deleteProjectCommand

Copy link
Copy Markdown
Collaborator

@aligator aligator left a comment

Choose a reason for hiding this comment

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

@dominikbraun

For me: LGTM

Comment thread cli/delete.go
}
}
return true
fmt.Fprint(os.Stderr, "Please confirm [y/N]: ")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Actually your code looks much cleaner. But I am not sure if @dominikbraun intends to remove the for loop.

I think the for loop is not necessary. So for me it looks ok :-)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You can throw it out if you want to.

Comment thread cli/delete.go
}
}
return true
fmt.Fprint(os.Stderr, "Please confirm [y/N]: ")
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You can throw it out if you want to.

@dominikbraun dominikbraun modified the milestones: timetrace v0.11.0, timetrace v0.12.0 Jul 2, 2021
@dominikbraun
Copy link
Copy Markdown
Owner

Thanks for solving this, @jwnpoh! Your change will be included in the next release.

@dominikbraun dominikbraun merged commit 85061d5 into dominikbraun:main Jul 2, 2021
@dominikbraun dominikbraun mentioned this pull request Jul 2, 2021
@dominikbraun dominikbraun modified the milestones: timetrace v0.12.0, timetrace v0.11.x Jul 2, 2021
@jwnpoh
Copy link
Copy Markdown
Contributor Author

jwnpoh commented Jul 2, 2021

Thrilled to have my first merged PR! Thanks for the opportunity!

@jwnpoh jwnpoh mentioned this pull request Jul 4, 2021
@dominikbraun
Copy link
Copy Markdown
Owner

This PR is included in timetrace v0.11.1.

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.

3 participants