code review 887045: Wiki codelab, complete with tests.

Wiki codelab, complete with tests.

[adg, 2010-04-07 06:08:27]

Hello r, rsc, gri (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc, 2010-04-07 06:14:54]

This looks like a good start.  Since I went through it the last time,
I'll leave the first round of comments to Rob.  My only presentation comment
is that nowhere on the page does it say that this is a code lab.

Russ

[adg, 2010-04-16 06:07:41]

Hello r, rsc, gri (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2010-04-19 22:26:19]

again, lots of niggly comments

http://codereview.appspot.com/887045/diff/26001/27016
File doc/codelab/wiki/wiki.html (right):

http://codereview.appspot.com/887045/diff/26001/27016#newcode15
doc/codelab/wiki/wiki.html:15: <li>Writing a function literal (or closure)</li>
again, a confusion between function literal and closure.  how about: Using
closures?

http://codereview.appspot.com/887045/diff/26001/27016#newcode31
doc/codelab/wiki/wiki.html:31: you don't have one handy, consider setting up a
Linux Virtual Machine (using
i dislike "consider" as an imperative verb.

http://codereview.appspot.com/887045/diff/26001/27016#newcode38
doc/codelab/wiki/wiki.html:38: Install Go as per the <a
href="http://golang.org/doc/install.html">Installation Instructions</a>.
"as per" is legalese

http://codereview.appspot.com/887045/diff/26001/27016#newcode65
doc/codelab/wiki/wiki.html:65: <code>fmt</code> and <code>ioutil</code> are
built-in packages that we'll be
sentence begins with a lower-case letter

http://codereview.appspot.com/887045/diff/26001/27016#newcode76
doc/codelab/wiki/wiki.html:76: the title and body. We declare it like this:
"Here, we..."  "We ..."   you're introducing the example twice.

http://codereview.appspot.com/887045/diff/26001/27016#newcode85
doc/codelab/wiki/wiki.html:85: (see <a
href="http://golang.org/doc/effective_go.html#slices">Effective Go</a>
s/s/S/

http://codereview.appspot.com/887045/diff/26001/27016#newcode86
doc/codelab/wiki/wiki.html:86: for more on slices)  We store <code>body</code>
as a <code>[]byte</code> rather
s/)/.)

http://codereview.appspot.com/887045/diff/26001/27016#newcode92
doc/codelab/wiki/wiki.html:92: This struct describes how page data will be
stored in memory. But what about
which struct?

http://codereview.appspot.com/887045/diff/26001/27016#newcode93
doc/codelab/wiki/wiki.html:93: persistent storage? Let's create a
<code>save</code> method on that struct:
which struct?

http://codereview.appspot.com/887045/diff/26001/27016#newcode93
doc/codelab/wiki/wiki.html:93: persistent storage? Let's create a
<code>save</code> method on that struct:
when asking a rhetorical question, you must answer it. it's not clear that the
next sentence is an answer. rewrite.

http://codereview.appspot.com/887045/diff/26001/27016#newcode101
doc/codelab/wiki/wiki.html:101: This function signature reads: "This is a method
named
maybe you should say method rather than function
also it should be
method's signature

http://codereview.appspot.com/887045/diff/26001/27016#newcode108
doc/codelab/wiki/wiki.html:108: This function will save the <code>page</code>'s
<code>body</code> to a text
here s/function/method/ for sure

http://codereview.appspot.com/887045/diff/26001/27016#newcode109
doc/codelab/wiki/wiki.html:109: file. For simplicity, we will use its
<code>title</code> as the filename.
its has no referent

http://codereview.appspot.com/887045/diff/26001/27016#newcode113
doc/codelab/wiki/wiki.html:113: Our function returns <code>os.Error</code>
because that is the return value of
s/Our function/The save method/

http://codereview.appspot.com/887045/diff/26001/27016#newcode115
doc/codelab/wiki/wiki.html:115: a file).  The function returns the error value,
to let the application handle
which function? your referents are very untidy

http://codereview.appspot.com/887045/diff/26001/27016#newcode121
doc/codelab/wiki/wiki.html:121: Passing <code>0600</code> as the third parameter
to <code>WriteFile</code>
it's not good to start a sentence with a gerund.  The subject of the sentence
should be the 0600 code itself.

http://codereview.appspot.com/887045/diff/26001/27016#newcode137
doc/codelab/wiki/wiki.html:137: <code>page</code>, and returns a pointer to it.
s/it/the page/

http://codereview.appspot.com/887045/diff/26001/27016#newcode143
doc/codelab/wiki/wiki.html:143: For simplicity's sake we won't make our pogram
handle the error right now,
s/pogram/program/

http://codereview.appspot.com/887045/diff/26001/27016#newcode151
doc/codelab/wiki/wiki.html:151: <code>*page</code> and <code>os.Error</code>:
it's odd that you throw away the error "right now" and then immediately handle
it. just write the correct code first and don't worry about showing off _.
you'll have another chance soon.

http://codereview.appspot.com/887045/diff/26001/27016#newcode168
doc/codelab/wiki/wiki.html:168: functionality:
"function to test functionality" is clumsy

how about "function to test what we've written".

http://codereview.appspot.com/887045/diff/26001/27016#newcode176
doc/codelab/wiki/wiki.html:176: After compiling and executing this code, a file
named TestPage.txt would be
should TestPage.txt be <code>?

http://codereview.appspot.com/887045/diff/26001/27016#newcode182
doc/codelab/wiki/wiki.html:182: You can compile and run the program like this:
assuming amd64. i think more people use 386.

http://codereview.appspot.com/887045/diff/26001/27016#newcode217
doc/codelab/wiki/wiki.html:217: This function will block until the program is
terminated.
it feels like this sentence explains the "Don't worry".  maybe the "Don't worry"
should be parenthesized.

http://codereview.appspot.com/887045/diff/26001/27016#newcode227
doc/codelab/wiki/wiki.html:227: An <code>http.Conn</code> is the server-side of
an HTTP connection; by writing
s/-side/ end/

http://codereview.appspot.com/887045/diff/26001/27016#newcode232
doc/codelab/wiki/wiki.html:232: An <code>http.Request</code> is the client's
HTTP request as a convenient
An http.Request is a data structure that represents the client Request.

http://codereview.appspot.com/887045/diff/26001/27016#newcode240
doc/codelab/wiki/wiki.html:240: For example, if you accessed this URL:
for an example of what?

read for flow. make sure each paragraph follows the preceding one without
unannounced changes of context

http://codereview.appspot.com/887045/diff/26001/27016#newcode272
doc/codelab/wiki/wiki.html:272: We re-slice <code>Path</code> with
<code>[6:]</code> to drop the first 6
the previous sentence was "this function"; now it's "we". be consistent, at
least locally

http://codereview.appspot.com/887045/diff/26001/27016#newcode273
doc/codelab/wiki/wiki.html:273: characters of the string. This is because it
will invariably begin
it has unclear referent

http://codereview.appspot.com/887045/diff/26001/27016#newcode314
doc/codelab/wiki/wiki.html:314: With our web server running, visit <code><a
s/our/your/

http://codereview.appspot.com/887045/diff/26001/27016#newcode316
doc/codelab/wiki/wiki.html:316: in your web browser. You should see a page
titled "test" containing the words
s/web //  not necessary

http://codereview.appspot.com/887045/diff/26001/27016#newcode324
doc/codelab/wiki/wiki.html:324: new handlers;
s/;/:/

http://codereview.appspot.com/887045/diff/26001/27016#newcode331
doc/codelab/wiki/wiki.html:331: First, we should add them to
<code>main()</code>:
s/should //

http://codereview.appspot.com/887045/diff/26001/27016#newcode339
doc/codelab/wiki/wiki.html:339: The function <code>editHandler</code> should
load the page
should again. why the change of tone?

http://codereview.appspot.com/887045/diff/26001/27016#newcode340
doc/codelab/wiki/wiki.html:340: (or, if it doesn't exist, create an empty
<code>page</code> struct),
this operation deserves a comment in the code

http://codereview.appspot.com/887045/diff/26001/27016#newcode350
doc/codelab/wiki/wiki.html:350: Fortunately, there's a better way.
s/Fortunately/Of course/

http://codereview.appspot.com/887045/diff/26001/27016#newcode395
doc/codelab/wiki/wiki.html:395: <code>edit.html</code> and return a
<code>*template.Template</code>.
you shouldn't ignore every error; it inculcates bad habits

http://codereview.appspot.com/887045/diff/26001/27016#newcode437
doc/codelab/wiki/wiki.html:437: Notice that we've used almost exactly the same
templating code in both
again, subject switch. the previous paragraph was you, now it's we.

http://codereview.appspot.com/887045/diff/26001/27016#newcode451
doc/codelab/wiki/wiki.html:451: The handlers are now shorter and more readable.
i'd s/more readable/simpler/

http://codereview.appspot.com/887045/diff/26001/27016#newcode458
doc/codelab/wiki/wiki.html:458: crash. This is because it ignores the error
return value from
it will crash because you wrote bad code. you could write good code and still
make a valid point here about improving the user's experience.

http://codereview.appspot.com/887045/diff/26001/27016#newcode497
doc/codelab/wiki/wiki.html:497: There are several places in our program where we
ignore errors.
earlier, when you ignore errors, you should either not do so or at least
acknowledge that you are and mention once or twice that you'll address the topic
later.

http://codereview.appspot.com/887045/diff/26001/27016#newcode515
doc/codelab/wiki/wiki.html:515: Already our decision to put this in a separate
function is paying off.
s/our/the/

http://codereview.appspot.com/887045/diff/26001/27016#newcode556
doc/codelab/wiki/wiki.html:556: panics if an error is encountered. This is
preferable here, as if the
confusing run-on sentence

http://codereview.appspot.com/887045/diff/26001/27016#newcode556
doc/codelab/wiki/wiki.html:556: panics if an error is encountered. This is
preferable here, as if the
what is This?

http://codereview.appspot.com/887045/diff/26001/27016#newcode570
doc/codelab/wiki/wiki.html:570: We then modify our <code>renderTemplate</code>
function to simply call
s/simply //

http://codereview.appspot.com/887045/diff/26001/27016#newcode639
doc/codelab/wiki/wiki.html:639: literals</a> provide a powerful means of
abstracting functionality,
s/,//

http://codereview.appspot.com/887045/diff/26001/27016#newcode640
doc/codelab/wiki/wiki.html:640: so we can do just that.
that can help us here.

http://codereview.appspot.com/887045/diff/26001/27016#newcode660
doc/codelab/wiki/wiki.html:660: func makeHandler(fn func (*http.Conn,
*http.Request, string)) http.HandlerFunc
why repeat the signature? just flow into the definition

http://codereview.appspot.com/887045/diff/26001/27016#newcode688
doc/codelab/wiki/wiki.html:688: 
probably worth explaining what this does. i mean, if you need to explain a
signature at the beginning of the tutorial this is surely in the 'needs
explanation' category

http://codereview.appspot.com/887045/diff/26001/27016#newcode690
doc/codelab/wiki/wiki.html:690: Wrap the handler functions with
<code>makeHandler</code> when we
another subject shift

[adg, 2010-04-20 10:01:21]

Hello r, rsc, gri (cc: golang-dev@googlegroups.com),

Please take another look.

[adg, 2010-04-23 06:01:04]

Hello r, rsc, gri (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2010-04-27 19:00:32]

much better. it's getting quite good

these are mostly minor comments.

http://codereview.appspot.com/887045/diff/41001/42021
File doc/codelab/wiki/wiki.html (right):

http://codereview.appspot.com/887045/diff/41001/42021#newcode67
doc/codelab/wiki/wiki.html:67: more packages to this <code>import</code>
statement.
s/statement/declaration/

http://codereview.appspot.com/887045/diff/41001/42021#newcode104
doc/codelab/wiki/wiki.html:104: takes as its receiver <code>p</code>, a pointer
to <code>page</code> . It takes
extra space before period

http://codereview.appspot.com/887045/diff/41001/42021#newcode110
doc/codelab/wiki/wiki.html:110: file. For simplicity, we will use the
<code>title</code> as the filename.
s/filename/file name/

http://codereview.appspot.com/887045/diff/41001/42021#newcode114
doc/codelab/wiki/wiki.html:114: The <code>save</code> method returns an
<code>os.Error</code> value, because
s/,//

http://codereview.appspot.com/887045/diff/41001/42021#newcode124
doc/codelab/wiki/wiki.html:124: The integer <code>0600</code>, passed as the
third parameter to
s/integer/octal integer constant/

(might as well hammer home that it's octal)

http://codereview.appspot.com/887045/diff/41001/42021#newcode139
doc/codelab/wiki/wiki.html:139: The function <code>loadPage</code> constructs
the filename from
s/filename/file name/

http://codereview.appspot.com/887045/diff/41001/42021#newcode147
doc/codelab/wiki/wiki.html:147: In <code>loadPage</code>, error isn't being
handled yet; the <code>_</code>
the "blank identifier" represented by the underscore (<code>_</code>) symbol

http://codereview.appspot.com/887045/diff/41001/42021#newcode154
doc/codelab/wiki/wiki.html:154: For example, the file might not exist.  Let's
modify the function to return
... might not exist. We should not ignore such errors.  Let's modify...

http://codereview.appspot.com/887045/diff/41001/42021#newcode201
doc/codelab/wiki/wiki.html:201: subtitute the 8's for 6's.)
substitute 6's for the 8's.

http://codereview.appspot.com/887045/diff/41001/42021#newcode252
doc/codelab/wiki/wiki.html:252: If you accessed this URL:
If you run this program and access the URL,

http://codereview.appspot.com/887045/diff/41001/42021#newcode256
doc/codelab/wiki/wiki.html:256: The program would return a page containing:
s/T/t/
s/return/present/

http://codereview.appspot.com/887045/diff/41001/42021#newcode285
doc/codelab/wiki/wiki.html:285: The <code>Path</code> is re-sliced with
<code>[6:]</code> to drop the first 6
this 6 has bugged me from the beginning. how about
const lenView = 6  // len("/view/")
or 
const lenView = len("/view/")

http://codereview.appspot.com/887045/diff/41001/42021#newcode328
doc/codelab/wiki/wiki.html:328: With your web server running, visit <code><a
With this web server running, a visit to ... should show a page titled...

http://codereview.appspot.com/887045/diff/41001/42021#newcode354
doc/codelab/wiki/wiki.html:354: and display an HTML form.
s/display/&s/

http://codereview.appspot.com/887045/diff/41001/42021#newcode422
doc/codelab/wiki/wiki.html:422: The <code>html</code> formatter escapes HTML
characters (eg, replacing
flow these two sentences together. maybe
before outputting it by escaping HTML characters...

http://codereview.appspot.com/887045/diff/41001/42021#newcode422
doc/codelab/wiki/wiki.html:422: The <code>html</code> formatter escapes HTML
characters (eg, replacing
"e.g." has periods, but even so i don't like latin abbrevs; they're pretentious.
use english.
such as replacing ....

http://codereview.appspot.com/887045/diff/41001/42021#newcode473
doc/codelab/wiki/wiki.html:473: redirect the client to the edit page so that
they can create it:
who are 'they'?
redirect the client to the edit page so the content may be created ?

http://codereview.appspot.com/887045/diff/41001/42021#newcode515
doc/codelab/wiki/wiki.html:515: crash.  A better solution is to handle the
errors, and return an error message
s/,//

http://codereview.appspot.com/887045/diff/41001/42021#newcode717
doc/codelab/wiki/wiki.html:717: Wrap the handler functions with
<code>makeHandler</code> in <code>main</code>,
s/Wrap/Now we can wrap/

http://codereview.appspot.com/887045/diff/41001/42021#newcode726
doc/codelab/wiki/wiki.html:726: Remove the calls to <code>getTitle</code> from
the handler functions,
s/Remove/Finally we remove

[adg, 2010-04-28 01:54:50]

Hello r, rsc, gri (cc: golang-dev@googlegroups.com),

Please take another look.

[r2, 2010-04-28 02:25:14]

LGTM

g stands for 'great'

-rob

On Apr 27, 2010, at 6:54 PM, adg@golang.org wrote:

> Hello r, rsc, gri (cc: golang-dev@googlegroups.com),
> 
> Please take another look.
> 
> 
> http://codereview.appspot.com/887045/show

[adg, 2010-04-28 02:36:47]

*** Submitted as http://code.google.com/p/go/source/detail?r=04f3893d07ba ***

Wiki codelab, complete with tests.

R=r, rsc, gri
CC=golang-dev
http://codereview.appspot.com/887045