Fix up undoStack.RunIfError #11
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
First, with Go 1.14, the use of `%w` in a Printf context other than Errorf causes a vet error. `%w` is a special trigger to Errorf to create a "wrapped" error. What you'd want to do instead is either `%s` or `%v`. However, given the nature of error strings, the behavior of simply concatenating them together to `w` seems inappropriate. If anything, they should at least occur on seperate lines, so Fprintln seems more appropriate. Second, RunIfError's comment and invocation in Exec made apparent another bug. As written, the undo stack would never actually execute. As directed, if you were to: defer undoStack.RunIfError(out, err) ...every value is evaluated at the point of `defer`, but the execution of the function is what is actually deferred. `err` will be `nil` there, and thus the undo stack is never actually executed. This simple fix there is to actually defer a closure. When an error is returned, it will then be observable from within the closure.
phamann
approved these changes
Mar 25, 2020
There was a problem hiding this comment.
👍 LGTM
Thank you for finding and fixing these issues, @jeddenlea
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First, with Go 1.14, the use of
%win a Printf context other than Errorfcauses a vet error.
%wis a special trigger to Errorf to create a"wrapped" error. What you'd want to do instead is either
%sor%v.However, given the nature of error strings, the behavior of simply
concatenating them together to
wseems inappropriate. If anything,they should at least occur on seperate lines, so Fprintln seems more
appropriate.
Second, RunIfError's comment and invocation in Exec made apparent
another bug. As written, the undo stack would never actually execute.
As directed, if you were to:
...every value is evaluated at the point of
defer, but the execution ofthe function is what is actually deferred.
errwill benilthere,and thus the undo stack is never actually executed.
This simple fix there is to actually defer a closure. When an error is
returned, it will then be observable from within the closure.