Issue 50970043: code review 50970043: cmd/go: run main package when no files are listed
|
Created:
10 years, 3 months ago by Dominik Honnef Modified:
9 years, 4 months ago Reviewers:
CC:
r, rsc, dave_cheney.net, minux1, gobot, golang-codereviews Visibility:
Public. |
Descriptioncmd/go: run main package when no files are listed
Additionally to the currently supported invocations, this change allows
running the current package with a bare "go run".
Fixes issue 5164.
Patch Set 1 #
Total comments: 6
Patch Set 2 : diff -r 7ecbc2b8ec97 https://code.google.com/p/go #Patch Set 3 : diff -r 7ecbc2b8ec97 https://code.google.com/p/go #Patch Set 4 : diff -r 7ecbc2b8ec97 https://code.google.com/p/go #
Total comments: 1
Patch Set 5 : diff -r 7ecbc2b8ec97 https://code.google.com/p/go #Patch Set 6 : diff -r 7ecbc2b8ec97 https://code.google.com/p/go #
Total comments: 1
Patch Set 7 : diff -r 7ecbc2b8ec97 https://code.google.com/p/go #Patch Set 8 : diff -r 7ecbc2b8ec97 https://code.google.com/p/go #
Total comments: 1
Patch Set 9 : diff -r 7ecbc2b8ec97 https://code.google.com/p/go #MessagesTotal messages: 31
Hello r, rsc (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
https://codereview.appspot.com/50970043/diff/1/src/cmd/go/run.go File src/cmd/go/run.go (right): https://codereview.appspot.com/50970043/diff/1/src/cmd/go/run.go#newcode19 src/cmd/go/run.go:19: A Go source file is defined to be a file ending in a literal ".go" suffix. this is not enough documentation for the change. also, i don't believe the expression in UsageLine; if it's right, i don't understand what it means.
Sign in to reply to this message.
https://codereview.appspot.com/50970043/diff/1/src/cmd/go/run.go File src/cmd/go/run.go (right): https://codereview.appspot.com/50970043/diff/1/src/cmd/go/run.go#newcode19 src/cmd/go/run.go:19: A Go source file is defined to be a file ending in a literal ".go" suffix. On 2014/01/12 16:34:31, r wrote: > this is not enough documentation for the change. also, i don't believe the > expression in UsageLine; if it's right, i don't understand what it means. Since I have listed valid invocations in the CL description, can you suggest a valid way of writing it? It's supposed to say that files and arguments are optional, but files, or ".", are required when specifying arguments.
Sign in to reply to this message.
https://codereview.appspot.com/50970043/diff/1/src/cmd/go/run.go File src/cmd/go/run.go (right): https://codereview.appspot.com/50970043/diff/1/src/cmd/go/run.go#newcode19 src/cmd/go/run.go:19: A Go source file is defined to be a file ending in a literal ".go" suffix. On 2014/01/12 16:39:12, Dominik Honnef wrote: > On 2014/01/12 16:34:31, r wrote: > > this is not enough documentation for the change. also, i don't believe the > > expression in UsageLine; if it's right, i don't understand what it means. > > Since I have listed valid invocations in the CL description, can you suggest a > valid way of writing it? It's supposed to say that files and arguments are > optional, but files, or ".", are required when specifying arguments. Not confidently because I am unclear about the rules. "Go run" will still be an oddball even after this change, assuming it goes in. One issue I am clear about is that it's not obvious that "arguments" refers to the arguments of the compiled binary as opposed to being for the go run command itself.
Sign in to reply to this message.
https://codereview.appspot.com/50970043/diff/1/src/cmd/go/run.go File src/cmd/go/run.go (right): https://codereview.appspot.com/50970043/diff/1/src/cmd/go/run.go#newcode19 src/cmd/go/run.go:19: A Go source file is defined to be a file ending in a literal ".go" suffix. Moreover, surely it's not the case that such arguments can be provided only if no .go files are mentioned explicitly. The documentation is wanting.
Sign in to reply to this message.
https://codereview.appspot.com/50970043/diff/1/src/cmd/go/run.go File src/cmd/go/run.go (right): https://codereview.appspot.com/50970043/diff/1/src/cmd/go/run.go#newcode19 src/cmd/go/run.go:19: A Go source file is defined to be a file ending in a literal ".go" suffix. On 2014/01/12 18:52:59, r wrote: > Moreover, surely it's not the case that such arguments can be provided only if > no .go files are mentioned explicitly. > > The documentation is wanting. Such arguments can be provided only if .go files or "." are mentioned. Let's just assume that my suggested UsageLine doesn't make any sense. These are the possible combinations of files and arguments: go run go run . go run foo.go go run foo.go -args go run . -args I agree that it's still an oddball, but the only way to make it truly look like build/install is to remove the current support for arguments.
Sign in to reply to this message.
https://codereview.appspot.com/50970043/diff/1/src/cmd/go/run.go File src/cmd/go/run.go (right): https://codereview.appspot.com/50970043/diff/1/src/cmd/go/run.go#newcode19 src/cmd/go/run.go:19: A Go source file is defined to be a file ending in a literal ".go" suffix. Please provide documentation.
Sign in to reply to this message.
Hello r@golang.org, rsc@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
Looks good as far as documentation is concerned. Thanks. For the code, I defer to Russ, who has strict ideas of what this feature should do.
Sign in to reply to this message.
On 2014/01/12 22:24:34, r wrote: > Looks good as far as documentation is concerned. Thanks. > > For the code, I defer to Russ, who has strict ideas of what this feature should > do. A frequent question on the mailing list/stack overflow/irc/twitter comes from people trying to use go run to work with multiple files. Worse, it encourages them to use file names, not packages for other commands, like go test something.go, which rarely works. Often suggesting the proper way to do things puts the nose of the OP out of joint and that isn't fun for anyone. I vote not to make go run easier to use as it will encourage people to delay learning how to properly structure Go code.
Sign in to reply to this message.
On Jan 31, 2014 12:04 AM, <dave@cheney.net> wrote: > A frequent question on the mailing list/stack overflow/irc/twitter comes > from people trying to use go run to work with multiple files. Worse, it > encourages them to use file names, not packages for other commands, like > go test something.go, which rarely works. Often suggesting the proper > way to do things puts the nose of the OP out of joint and that isn't fun > for anyone. > > I vote not to make go run easier to use as it will encourage people to > delay learning how to properly structure Go code. i agree.
Sign in to reply to this message.
On 2014/01/31 05:33:31, minux wrote: > On Jan 31, 2014 12:04 AM, <mailto:dave@cheney.net> wrote: > > A frequent question on the mailing list/stack overflow/irc/twitter comes > > from people trying to use go run to work with multiple files. Worse, it > > encourages them to use file names, not packages for other commands, like > > go test something.go, which rarely works. Often suggesting the proper > > way to do things puts the nose of the OP out of joint and that isn't fun > > for anyone. > > > > I vote not to make go run easier to use as it will encourage people to > > delay learning how to properly structure Go code. > i agree. On the other hand, one might argue that if go run worked without file names, it might encourage people to use it like the other go tools, i.e. use it on packages, not files, especially if some other documentation were to be adjusted as well. Of course what remains are people treating Go like a scripting language. I'm not a huge fan of the way people use go run right now, but I made these changes because Russ wanted them at the time, and because I think it might become less harmless if it worked like the other tools because the other solution, removing it altogether isn't really an option.
Sign in to reply to this message.
PTAL Trying to get the status in the dashboard right. Also awaiting a final verdict on the desirability of this feature (Russ?)
Sign in to reply to this message.
The idea is fine, but the "." is awkward. Let's remove it. (If "go run ." works, then why does "go run path/to/cmd" not work?) https://codereview.appspot.com/50970043/diff/60001/src/cmd/go/run.go File src/cmd/go/run.go (right): https://codereview.appspot.com/50970043/diff/60001/src/cmd/go/run.go#newcode63 src/cmd/go/run.go:63: // Allowing this would lead to ambiguous invocations such as Making the command line less regular just to help someone with a typo is a pretty weak argument. I'd like to remove all mention of ".". If someone says 'go run foo.g', then it's not a go file.
Sign in to reply to this message.
FWIW, I did think about making the first argument always be an import path, generalizing "go run ." to "go run path/to/cmd", but I don't think that's something we want to encourage, and we can cut it off from the start.
Sign in to reply to this message.
R=r@golang.org (assigned by r@golang.org)
Sign in to reply to this message.
R=rsc@golang.org (assigned by r@golang.org)
Sign in to reply to this message.
PTAL "." is gone. The only way to run the current package without specifying any files is via "go run". This means that arguments cannot be passed to the program when not specifying Go files (a side effect is that `go run` can only be used with cgo files when not passing arguments.)
Sign in to reply to this message.
https://codereview.appspot.com/50970043/diff/100001/src/cmd/go/run.go File src/cmd/go/run.go (right): https://codereview.appspot.com/50970043/diff/100001/src/cmd/go/run.go#newcode62 src/cmd/go/run.go:62: if len(cmdArgs) > 0 && len(files) == 0 { Please delete this. I see no reason for this.
Sign in to reply to this message.
Hello r@golang.org, rsc@golang.org, dave@cheney.net, minux.ma@gmail.com, gobot@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/50970043/diff/140001/src/cmd/go/run.go File src/cmd/go/run.go (right): https://codereview.appspot.com/50970043/diff/140001/src/cmd/go/run.go#newcode25 src/cmd/go/run.go:25: verbatim to the executable. To avoid ambiguities in flag parsing, at Is this still true? What are the ambiguities? I still don't see them. If not, please remove this sentence, regenerate doc.go, and update the CL description.
Sign in to reply to this message.
On 2014/03/03 16:52:09, rsc wrote: > https://codereview.appspot.com/50970043/diff/140001/src/cmd/go/run.go > File src/cmd/go/run.go (right): > > https://codereview.appspot.com/50970043/diff/140001/src/cmd/go/run.go#newcode25 > src/cmd/go/run.go:25: verbatim to the executable. To avoid ambiguities in flag > parsing, at > Is this still true? What are the ambiguities? I still don't see them. > If not, please remove this sentence, regenerate doc.go, and update the CL > description. It's not technically an ambiguity. One cannot use `go run -foo` to run the current package and pass `-foo` as an argument to it (compare with `go run *.go -foo`). It's not an ambiguity, because `go run -foo` will correctly be rejected, but there should be a sentence saying that passing arguments to the program isn't possible when not specifying any files. How would you word it? "Passing arguments requires that at least one Go source be named"?
Sign in to reply to this message.
On Mar 3, 2014 11:55 AM, <dominik.honnef@gmail.com> wrote: > https://codereview.appspot.com/50970043/diff/140001/src/cmd/go/run.go#newcode25 >> >> src/cmd/go/run.go:25: verbatim to the executable. To avoid ambiguities > > in flag >> >> parsing, at >> Is this still true? What are the ambiguities? I still don't see them. >> If not, please remove this sentence, regenerate doc.go, and update the > CL >> description. > It's not technically an ambiguity. One cannot use `go run -foo` to run > the current package and pass `-foo` as an argument to it (compare with > `go run *.go -foo`). It's not an ambiguity, because `go run -foo` will > correctly be rejected, but there should be a sentence saying that why reject it? i'd expect "go run -- -foo" to work for this case. > passing arguments to the program isn't possible when not specifying any > files. > > How would you word it? "Passing arguments requires that at least one Go > source be named"?
Sign in to reply to this message.
Updated the CL description, regenerated the documentation. A lot of churn for what turns out to be a trivial change... (I was not aware that Go's flag parsing supported `--`)
Sign in to reply to this message.
-- doesn't really do what you want either. I'd like to think more about this.
Sign in to reply to this message.
I really like 'go run foo' meaning run the current directory's command with argument foo, but the ambiguity when foo ends in .go is really bothering me, which is why I haven't said anything. We could require 'go run . foo' instead, but that's a little odd and is too suggestive of 'go run some/import/path foo', which I definitely don't want: go is not a scripting language. Since none of the options really feels right, I'd like to postpone this CL until after Go 1.3 so we don't have to commit to anything right now. Thanks for working on this, and I'm sorry the resolution isn't nicer.
Sign in to reply to this message.
Russ, should I re-send this CL for the new code review system, or is there no point in doing so?
Sign in to reply to this message.
On 2014/12/18 09:39:43, Dominik Honnef wrote: > Russ, should I re-send this CL for the new code review system, or is there no > point in doing so? Without a better design, it's not worth sending. And I don't see a better design. All the possibilities that have been raised have problems.
Sign in to reply to this message.
I will add that if you want to write a separate script to do this, it is not too
hard:
#!/bin/bash
go run $(go list -f '{{join .GoFiles " "}} {{join .CgoFiles " "}}') "$@"
Sign in to reply to this message.
On 2014/12/18 15:39:58, rsc wrote:
> I will add that if you want to write a separate script to do this, it is not
too
> hard:
>
> #!/bin/bash
> go run $(go list -f '{{join .GoFiles " "}} {{join .CgoFiles " "}}') "$@"
That script won't actually work for most packages that use cgo, as CgoFiles
doesn't include .c, .h, ... files – and even if one includes these in the list,
`go run` stops processing the list of files as soon as it finds one that doesn't
end with ".go"
I haven't come up with a better design yet, either, so I'll drop the CL.
Sign in to reply to this message.
R=close To the author of this CL: The Go project has moved to Gerrit Code Review. If this CL should be continued, please see the latest version of https://golang.org/doc/contribute.html for instructions on how to set up Git and the Go project's Gerrit codereview plugin, and then create a new change with your current code. If there has been discussion on this CL, please give a link to it (golang.org/cl/50970043 is best) in the description in your new CL. Thanks very much.
Sign in to reply to this message.
|
