Issue 108140044: code review 108140044: go.talks: Organizing Go Code (Closed)
Descriptiongo.talks: Organizing Go Code
Patch Set 1 #Patch Set 2 : diff -r c636a8a8316f https://code.google.com/p/go.talks #
Total comments: 35
Patch Set 3 : diff -r c636a8a8316f https://code.google.com/p/go.talks #Patch Set 4 : diff -r ab43b4d3430ba021b95822cb22875486f39b5c09 https://code.google.com/p/go.talks #
MessagesTotal messages: 11
https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide File 2014/organizeio.slide (right): https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:11: All Go source is part of a package, Shouldn't this end with a period instead of a comma? Perhaps I'm misreading this as a sentence instead of a clause because of the capitalization on the next line. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:274: * Naming Perhaps mention exported vs unexported names in this section?
Sign in to reply to this message.
https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide File 2014/organizeio.slide (right): https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:312: Use the shortest name you can. seems too extreme. Perhaps: Use the shortest name that makes sense. Use the shortest sensible name. Use the shortest comprehensible name. Use the shortest readable name.
Sign in to reply to this message.
https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide File 2014/organizeio.slide (right): https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:21: } could be an executable program with a Run button. (why not?) .play subdirectory/hello.go https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:23: For very small programs, main is the only package you need to write. s/main/`&`/ https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:66: Don't use underscores, they make package names long. give bad example and good example https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:72: The name of a package is part of the its types and function s/the // s/types/type/ https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:73: names. On its own, type `Buffer` is ambigious. But users see: ambiguous https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:79: Name for users. Choose good names for users. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:135: The `GOPATH` environment variable tells the Go tool about your workspace. tells the Go tool where your workspace is located. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:139: The go get command fetches source repositories from the internet. and places them in your workspace. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:141: Package paths matter to the Go tool. Using "github.com/user/repo/mypkg" maybe `"github...."` https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:178: No `Makefile`, no `build.xml`. you didn't `` Makefile last time. maybe you should have. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:216: This lets you move around using the Go tool's path names: no one likes my CDPATH thing, which is nice too. export CDPATH=.:$HOME:$GOROOT/src/pkg/:$GOPATH/src/code.google.com/p:$GOPATH/src/git... https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:219: /tmp/gows/src/github.com/golang/lint /tmp/gows? https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:293: These are good names. strange slide. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:312: Use the shortest name you can. use the shortest name that carries the right amount of information in its context https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:318: // Join concatenates the elements of a to create a single string. "a to" choose a better example (or change the variable name)
Sign in to reply to this message.
https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide File 2014/organizeio.slide (right): https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:11: All Go source is part of a package, On 2014/06/24 20:59:35, cmang wrote: > Shouldn't this end with a period instead of a comma? Perhaps I'm misreading > this as a sentence instead of a clause because of the capitalization on the next > line. Done. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:21: } On 2014/06/24 21:13:51, r wrote: > could be an executable program with a Run button. (why not?) > > .play subdirectory/hello.go Done. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:23: For very small programs, main is the only package you need to write. On 2014/06/24 21:13:51, r wrote: > s/main/`&`/ Done. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:66: Don't use underscores, they make package names long. On 2014/06/24 21:13:50, r wrote: > give bad example and good example Done. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:72: The name of a package is part of the its types and function On 2014/06/24 21:13:51, r wrote: > s/the // > s/types/type/ Done. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:73: names. On its own, type `Buffer` is ambigious. But users see: On 2014/06/24 21:13:51, r wrote: > ambiguous Done. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:79: Name for users. On 2014/06/24 21:13:51, r wrote: > Choose good names for users. Done. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:135: The `GOPATH` environment variable tells the Go tool about your workspace. On 2014/06/24 21:13:51, r wrote: > tells the Go tool where your workspace is located. Done. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:139: The go get command fetches source repositories from the internet. On 2014/06/24 21:13:51, r wrote: > and places them in your workspace. Done. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:141: Package paths matter to the Go tool. Using "github.com/user/repo/mypkg" On 2014/06/24 21:13:51, r wrote: > maybe `"github...."` Done. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:178: No `Makefile`, no `build.xml`. On 2014/06/24 21:13:50, r wrote: > you didn't `` Makefile last time. maybe you should have. Done. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:216: This lets you move around using the Go tool's path names: On 2014/06/24 21:13:51, r wrote: > no one likes my CDPATH thing, which is nice too. > export > http://CDPATH=.:$HOME:$GOROOT/src/pkg/:$GOPATH/src/code.google.com/p:$GOPATH/... > The CDPATH example is above this. I didn't include .:$HOME just to keep the slide as short as possible. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:219: /tmp/gows/src/github.com/golang/lint On 2014/06/24 21:13:50, r wrote: > /tmp/gows? it's the example used earlier https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:274: * Naming On 2014/06/24 20:59:35, cmang wrote: > Perhaps mention exported vs unexported names in this section? it's mentioned above. i'll bring it up in discussion. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:293: These are good names. On 2014/06/24 21:13:51, r wrote: > strange slide. rewritten https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:312: Use the shortest name you can. On 2014/06/24 21:13:51, r wrote: > use the shortest name that carries the right amount of information in its > context Done. https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... 2014/organizeio.slide:318: // Join concatenates the elements of a to create a single string. On 2014/06/24 21:13:50, r wrote: > "a to" > choose a better example (or change the variable name) Done.
Sign in to reply to this message.
On 2014/06/24 22:23:37, crawshaw wrote: > https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide > File 2014/organizeio.slide (right): > > https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... > 2014/organizeio.slide:11: All Go source is part of a package, > On 2014/06/24 20:59:35, cmang wrote: > > Shouldn't this end with a period instead of a comma? Perhaps I'm misreading > > this as a sentence instead of a clause because of the capitalization on the > next > > line. > > Done. > > https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... > 2014/organizeio.slide:21: } > On 2014/06/24 21:13:51, r wrote: > > could be an executable program with a Run button. (why not?) > > > > .play subdirectory/hello.go > > Done. > > https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... > 2014/organizeio.slide:23: For very small programs, main is the only package you > need to write. > On 2014/06/24 21:13:51, r wrote: > > s/main/`&`/ > > Done. > > https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... > 2014/organizeio.slide:66: Don't use underscores, they make package names long. > On 2014/06/24 21:13:50, r wrote: > > give bad example and good example > > Done. > > https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... > 2014/organizeio.slide:72: The name of a package is part of the its types and > function > On 2014/06/24 21:13:51, r wrote: > > s/the // > > s/types/type/ > > Done. > > https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... > 2014/organizeio.slide:73: names. On its own, type `Buffer` is ambigious. But > users see: > On 2014/06/24 21:13:51, r wrote: > > ambiguous > > Done. > > https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... > 2014/organizeio.slide:79: Name for users. > On 2014/06/24 21:13:51, r wrote: > > Choose good names for users. > > Done. > > https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... > 2014/organizeio.slide:135: The `GOPATH` environment variable tells the Go tool > about your workspace. > On 2014/06/24 21:13:51, r wrote: > > tells the Go tool where your workspace is located. > > Done. > > https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... > 2014/organizeio.slide:139: The go get command fetches source repositories from > the internet. > On 2014/06/24 21:13:51, r wrote: > > and places them in your workspace. > > Done. > > https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... > 2014/organizeio.slide:141: Package paths matter to the Go tool. Using > "github.com/user/repo/mypkg" > On 2014/06/24 21:13:51, r wrote: > > maybe `"github...."` > > Done. > > https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... > 2014/organizeio.slide:178: No `Makefile`, no `build.xml`. > On 2014/06/24 21:13:50, r wrote: > > you didn't `` Makefile last time. maybe you should have. > > Done. > > https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... > 2014/organizeio.slide:216: This lets you move around using the Go tool's path > names: > On 2014/06/24 21:13:51, r wrote: > > no one likes my CDPATH thing, which is nice too. > > export > > > http://CDPATH=.:$HOME:$GOROOT/src/pkg/:$GOPATH/src/code.google.com/p:$GOPATH/... > > > > The CDPATH example is above this. I didn't include .:$HOME just to keep the > slide as short as possible. > > https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... > 2014/organizeio.slide:219: /tmp/gows/src/github.com/golang/lint > On 2014/06/24 21:13:50, r wrote: > > /tmp/gows? > > it's the example used earlier > > https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... > 2014/organizeio.slide:274: * Naming > On 2014/06/24 20:59:35, cmang wrote: > > Perhaps mention exported vs unexported names in this section? > > it's mentioned above. i'll bring it up in discussion. > > https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... > 2014/organizeio.slide:293: These are good names. > On 2014/06/24 21:13:51, r wrote: > > strange slide. > > rewritten > > https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... > 2014/organizeio.slide:312: Use the shortest name you can. > On 2014/06/24 21:13:51, r wrote: > > use the shortest name that carries the right amount of information in its > > context > > Done. > > https://codereview.appspot.com/108140044/diff/20001/2014/organizeio.slide#new... > 2014/organizeio.slide:318: // Join concatenates the elements of a to create a > single string. > On 2014/06/24 21:13:50, r wrote: > > "a to" > > choose a better example (or change the variable name) > > Done. was this submitted?
Sign in to reply to this message.
On 2014/07/11 23:44:32, campoy wrote: > was this submitted? not yet. any other comments?
Sign in to reply to this message.
On 2014/07/12 16:36:29, crawshaw wrote: > On 2014/07/11 23:44:32, campoy wrote: > > was this submitted? > > not yet. any other comments? Are we good with this?
Sign in to reply to this message.
On 2014/09/16 19:09:00, campoy wrote: > On 2014/07/12 16:36:29, crawshaw wrote: > > On 2014/07/11 23:44:32, campoy wrote: > > > was this submitted? > > > > not yet. any other comments? > > Are we good with this? I think so. If you want me to submit it, I need an lgtm.
Sign in to reply to this message.
LGTM On Wed, Sep 17, 2014 at 11:24 AM, <crawshaw@golang.org> wrote: > On 2014/09/16 19:09:00, campoy wrote: > >> On 2014/07/12 16:36:29, crawshaw wrote: >> > On 2014/07/11 23:44:32, campoy wrote: >> > > was this submitted? >> > >> > not yet. any other comments? >> > > Are we good with this? >> > > I think so. If you want me to submit it, I need an lgtm. > > https://codereview.appspot.com/108140044/ >
Sign in to reply to this message.
Hello cmang@golang.org, sameer@google.com, r@golang.org, campoy@google.com (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.talks
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=312044522b9c&repo=talks *** go.talks: Organizing Go Code LGTM=campoy R=cmang, sameer, r, campoy CC=golang-codereviews https://codereview.appspot.com/108140044
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
