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

x/playground: support txtar input files #32040

Open
bradfitz opened this issue May 14, 2019 · 8 comments

Comments

Projects
None yet
5 participants
@bradfitz
Copy link
Member

commented May 14, 2019

(Pulling out of related third-party imports bug #31944)

This is a tracking bug to support multiple input files.

We'll start with txtar format, and use txtar behind the scenes when saving shared snippets. But we might add web UI to have separate <textarea> boxes later.

@gopherbot gopherbot added this to the Unreleased milestone May 14, 2019

@gopherbot

This comment has been minimized.

Copy link

commented May 14, 2019

Change https://golang.org/cl/177043 mentions this issue: playground: support multiple input files in txtar format

@dmitshur

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@bradfitz The current implementation of splitFiles in CL 177043 is described as:

If src isn't in txtar format, it returns a file named "prog.go".

Have you considered a behavior where the first file doesn't need to specify the -- prog.go -- header? E.g.:

package main
func main() {println(foo)}

-- prog2.go --
package main
const foo = "this is okay"

The txtar archive comment, if present, becomes prog.go content.

Or, a more important/common situation if as I understand correctly that future shared snippets (that import third-party packages?) will have a go.mod file automatically appended:

package main

func main() { [...] }

-- go.mod --
module [...]

The motivation is similar to how v1 of modules doesn't require a /v1 suffix, only v2+ does. Most snippets are just one .go file, and the second file may end up being just an automatically generated go.mod file. It seems unfortunate to require -- prog.go -- header on top of every snippet because of that.

The advantage of the current requirement of first header is that it's more visible when txtar is used or not used.

@dmitshur

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Oh, I see now that the code already does that. I missed that there's no return in the if v := bytes.TrimSpace(a.Comment); len(v) > 0 { block.

Edit: The function documentation was misleading; I'll leave a comment in code review.

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

It even has tests! ;)

@dmitshur

This comment has been minimized.

Copy link
Member

commented May 14, 2019

I missed the test case because it wasn't in between the two other test cases where I expected to see it. I left a comment about that too. 😛

gopherbot pushed a commit to golang/playground that referenced this issue May 15, 2019

playground: support multiple input files in txtar format
Updates golang/go#32040
Updates golang/go#31944 (Notably, you can now include a go.mod file)

Change-Id: I56846e86d3d98fdf4cac388b5b284dbc187e3b36
Reviewed-on: https://go-review.googlesource.com/c/playground/+/177043
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented May 15, 2019

Change https://golang.org/cl/177421 mentions this issue: playground: format go.mod files; fix paths in formatting errors

gopherbot pushed a commit to golang/playground that referenced this issue May 15, 2019

playground: format go.mod files; fix paths in formatting errors
Previously, handleFmt applied gofmt/goimports formatting to .go files
only. This change makes it apply formatting to go.mod files as well.
The cmd/go/internal/modfile package (well, a non-internal copy thereof)
is used to perform the go.mod file formatting.

Add test cases for error messages, and fix some cases where the paths
weren't accurate.

Detect when the error was returned by format.Source and needs to be
prefixed by using the fixImports variable instead of checking for the
presence of the prefix. This makes the code simpler and more readable.

Replace old fs.m[f] usage with fs.Data(f) in fmt.go and txtar_test.go.

Updates golang/go#32040
Updates golang/go#31944

Change-Id: Iefef7337f962914817558bcf0c622a952160ac44
Reviewed-on: https://go-review.googlesource.com/c/playground/+/177421
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@ianthehat

This comment has been minimized.

Copy link

commented May 20, 2019

It would be really cool if you could map the files into the sandbox so you could have data files in the set, allowing things like
https://play.golang.org/p/EbMXTpZSndB

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

@ianthehat, I plan to add that, but doing it with the nacl backend would involve doing some wasted work, as I'd need to do the whole zip thing that we do for tests now with misc/nacl/mkzip.go and make some syscall changes to shove it in.

I'd rather do it once we've moved the playground to linux/amd64, and then just write the files to "disk" (tmpfs) and access them directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.