all: add support for txtar extension and prefer it#159
all: add support for txtar extension and prefer it#159rogpeppe merged 1 commit intorogpeppe:masterfrom
Conversation
rogpeppe
left a comment
rogpeppe
left a comment
There was a problem hiding this comment.
LGTM with a couple of minor suggestions, thanks!
| name = strings.TrimSuffix(name, ".txt") | ||
| case strings.HasSuffix(name, ".txtar"): | ||
| name = strings.TrimSuffix(name, ".txtar") | ||
| case info.IsDir(): |
There was a problem hiding this comment.
Why are we allowing a directory when we weren't before? Maybe worth a comment?
There was a problem hiding this comment.
we did continue the loop in the case of info.IsDir() before; see the deleted line. I essentially inverted the !foo && !bar with a break into a foo || bar with an else (the default case) which breaks.
one logical difference is that the old code would accept module_version.txt/ as a directory, happily stripping the .txt suffix. I think that was a coding mistake, because that feature was never documented - the docs only refer to module_version.txt as a file, and module_version/ as a directory.
There was a problem hiding this comment.
Sorry, I misread the previous logic. All LGTM.
For backwards compatibility, both testscript and goproxytest, which used to glob on `*.txt`, now look for both file extensions. Note that this required a bit of a refactor in testscript, as we cannot use a single glob expression to accomplish this. Code which produces files, such as txtar-addmod, now produces `*.txtar` rather than `*.txt`. Similarly, the public docs now use `*.txtar` too. Note that we leave many txt files in tests untouched; it's unnecessary to change them given the backwards compatibility, and it has zero benefit to the user as they aren't public. Moreover, the diff churn would make this patch harder to review. If a future version of go-internal only supports txtar extensions, then it could replace all of those extensions accordingly. Fixes rogpeppe#126.
|
@rogpeppe are you OK with me merging this? :) |
2 participants
(see commit message)
Fixes #126.