Skip to content
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

testscript: add support for the txtar file extension #126

Closed
myitcv opened this issue Feb 4, 2021 · 5 comments · Fixed by #159
Closed

testscript: add support for the txtar file extension #126

myitcv opened this issue Feb 4, 2021 · 5 comments · Fixed by #159
Labels
enhancement New feature or request

Comments

@myitcv
Copy link
Collaborator

myitcv commented Feb 4, 2021

Currently, testscript is hard-coded to only look for .txt files.

This is unfortunate, because .txtar is a more descriptive extension that would better lend itself to automatic syntax highlighting (on GitHub) etc.

Opening this issue to capture the options for changing/extending this:

  1. switch the default to search for .txt and .txtar
  2. make it configurable via testscript.Params
  3. ...
@myitcv myitcv added the enhancement New feature or request label Feb 4, 2021
@mvdan
Copy link
Collaborator

mvdan commented Feb 4, 2021

I personally vote for option 1 - to just change the behavior. I'd be incredibly surprised if it broke any existing programs/tests. I understand the current semantics to be "load all txtar files in the provided directory", not "load all files in the provided directory with the txt extension". Arguably, using .txt instead of .txtar was a bit of a mistake early on.

@mvdan
Copy link
Collaborator

mvdan commented Jun 10, 2022

As a bit of an update: I propose that the tools use *.txtar by default - for example, txtar-addmod would now produce a *.txtar rather than a *.txt file. All code that globs would look for both extensions - which is arguably a bit cumbersome, as we can't do something like txt(ar)? with globbing, so we'd have to do our own little wrapper to read a directory and filter by either extension. But it's doable.

Then none of the existing users should break, new code would naturally gravitate towards using *.txtar, and we could start using it in our projects without needing new API.

@rogpeppe @myitcv thoughts? I'd like to make this transition happen in my repos and am happy to implement it here :)

@myitcv
Copy link
Collaborator Author

myitcv commented Jun 10, 2022

I'm happy with that suggestion.

@rogpeppe
Copy link
Owner

Yeah, I've thought for a while that .txtar would be a better extension.

@mvdan mvdan changed the title testscript: add support for additional file patterns testscript: add support for the txtar file extension Jun 13, 2022
mvdan added a commit to mvdan/go-internal that referenced this issue Jun 13, 2022
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.
@mvdan
Copy link
Collaborator

mvdan commented Jun 13, 2022

PR ready :)

mvdan added a commit to mvdan/go-internal that referenced this issue Jun 13, 2022
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.
mvdan added a commit to mvdan/go-internal that referenced this issue Jun 17, 2022
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 pushed a commit that referenced this issue Jun 24, 2022
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 #126.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants