-
Notifications
You must be signed in to change notification settings - Fork 70
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
all: add support for txtar extension and prefer it #159
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we allowing a directory when we weren't before? Maybe worth a comment?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? :) |
(see commit message)
Fixes #126.