Skip to content

Conversation

jwnpoh
Copy link
Contributor

@jwnpoh jwnpoh commented Jul 4, 2021

This PR addresses:

  1. typos in the --revert help messages, change it's to its.
  2. My earlier PR timetrace delete commands: Default to no when asking for confirmation #147 failed to include the check for --yes flag in deleteProjectCommand. This PR fixes that.

Copy link
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.

Hi, thanks for the PR.

I have just a small change request.

cli/delete.go Outdated
Comment on lines 58 to 63
if !confirmed {
if !askForConfirmation() {
out.Info("Project NOT deleted.")
return
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't that be a bit nicer code? Avoid stacked ifs if possible and feasible.

Suggested change
if !confirmed {
if !askForConfirmation() {
out.Info("Project NOT deleted.")
return
}
}
if !confirmed && !askForConfirmation() {
out.Info("Project NOT deleted.")
return
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that makes sense. In fact, I was merely mindlessly copying the if-logic from the deleteRecordCommand and did not even think that there was a cleaner way to do it. I will submit another commit with an edit to deleteRecordCommand as well.

Copy link
Contributor Author

@jwnpoh jwnpoh Jul 5, 2021

Choose a reason for hiding this comment

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

While discussing code style, I also refactored the if options.Revert checks to remove the else statement for cleaner code, following the advice from https://medium.com/@matryer/line-of-sight-in-code-186dd7cdea88.

Please have a look at the new commit and let me know if that's alright!

@dominikbraun dominikbraun added this to the timetrace v0.11.x milestone Jul 5, 2021
Copy link
Owner

@dominikbraun dominikbraun left a comment

Choose a reason for hiding this comment

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

Nice! The best else-blocks are the else-blocks that don't exist 😎

This fix will be included in the next patch release later today.

@dominikbraun dominikbraun merged commit 65d160e into dominikbraun:main Jul 5, 2021
@dominikbraun
Copy link
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