Conversation
aligator
left a comment
There was a problem hiding this comment.
Hi, thanks for the PR.
I have just a small change request.
| if !confirmed { | ||
| if !askForConfirmation() { | ||
| out.Info("Project NOT deleted.") | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
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.
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
left a comment
There was a problem hiding this comment.
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:
--reverthelp messages, changeit'stoits.--yesflag indeleteProjectCommand. This PR fixes that.