-
Notifications
You must be signed in to change notification settings - Fork 80
Readme #155
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
Readme #155
Conversation
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.
Hi, thanks for the PR.
I have just a small change request.
cli/delete.go
Outdated
if !confirmed { | ||
if !askForConfirmation() { | ||
out.Info("Project NOT deleted.") | ||
return | ||
} | ||
} |
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.
Wouldn't that be a bit nicer code? Avoid stacked ifs if possible and feasible.
if !confirmed { | |
if !askForConfirmation() { | |
out.Info("Project NOT deleted.") | |
return | |
} | |
} | |
if !confirmed && !askForConfirmation() { | |
out.Info("Project NOT deleted.") | |
return | |
} |
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.
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.
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.
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!
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.
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.
This PR is included in timetrace v0.11.1. |
This PR addresses:
--revert
help messages, changeit's
toits
.--yes
flag indeleteProjectCommand
. This PR fixes that.