testscript: port "mv" and "! cmp" from upstream#168
Merged
mvdan merged 1 commit intorogpeppe:masterfrom Aug 4, 2022
Merged
Conversation
rogpeppe
requested changes
Aug 3, 2022
Owner
rogpeppe
left a comment
rogpeppe
left a comment
There was a problem hiding this comment.
LGTM, thanks, except that the new functionality needs to be added to the docs.
mvdan
force-pushed
the
153-port
branch
2 times, most recently
from
16cbd15 to
24c64e7
Compare
Collaborator
Author
|
PTAL. The way gopls ran 1.19's gofmt, it shifted quite a bit of whitespace in doc.go, which I think we want anyway. To ease your review, ignore whitespace changes: https://github.com/rogpeppe/go-internal/pull/168/files?w=1 |
rogpeppe
requested changes
Aug 3, 2022
Owner
rogpeppe
left a comment
rogpeppe
left a comment
There was a problem hiding this comment.
LGTM modulo one last trivial thing.
They used args[0], which is the first argument to the command, rather than the command name itself: > gofumpt foo.go FAIL: [...] use 'exec foo.go' rather than 'foo.go' (because RequireExplicitExec is enabled) I believe I introduced this regression when refactoring the pull request due to Roger's review, as I moved the code out of TestMain, where args[0] was still the name of the command to be run. Note that I can't easily add a regression test here, because the testscript command being used in the tests is not a top-level command run as a separate process, so it does not produce stdout nor stderr. The change seems easy enough to review manually, and our tests don't strictly check all error messages anyway. While here, remove the unnecessary use of -verbose.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
(see commit message)
Fixes #153.