code review 118170043: go-tour: add Stringer, Reader discussions and exercises...

go-tour: add Stringer, Reader discussions and exercises; other tweaks

[adg, 2014-07-29 01:10:23]

Hello campoy (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go-tour

[campoy, 2014-07-29 21:43:04]

https://codereview.appspot.com/118170043/diff/40001/content/methods.article
File content/methods.article (right):

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:17: You can declare a method on _any_ named type in your
package, not just struct types.
I'm not sure about the "named" reference here. It might make people wonder what
that means.

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:64: * Stringers
Adding this section makes me think we should have at least a link to the
printing format, or at least mention what %v does.

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:66: One of the most ubiquitous interfaces is `Stringer`
defined by the `fmt` package.
Missing comma after `Stringer`?

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:66: One of the most ubiquitous interfaces is `Stringer`
defined by the `fmt` package.
Should Stringer be a link to its doc?

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:73: (and many others) look for this interface for
pretty-printing values.
I'd just say "to print" values. Pretty-printing is pretty subjective in this
case.

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:97: pretty-printing values.)
/pretty-/d

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:105: }
add extra line using i

fmt.Printf("converted number: %v\n", i)

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:107: A nil error denotes success; a non-nil error
denotes failure.
`error`

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:136: which represents a stream of binary data.
a read-only stream of data.

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:138: The Go standard library contains
[[http://golang.org/search?q=Read#Global][many implementations]] of these
interfaces, from files to network connections to compressors to ciphers.
The from to construction with multiple "to"s reads awkward.

, including files, network connections, compressors, ciphers, and others.

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:161: * Exercise: rot13 Reader
ROT13?

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:165: For example, the
[[http://golang.org/pkg/compress/gzip/#NewReader][gzip.NewReader]] function
takes an `io.Reader` (a stream of gzipped data) and returns a `*gzip.Reader`
that also implements `io.Reader` (a stream of the decompressed data).
if you say gzipped, should you say gunzipped?

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:229: *Note*: the `Rectangle` return value of the
`Bounds` method is actually an
[[http://golang.org/pkg/image/#Rectangle][`image.Rectangle`]], as the
declaration is inside package `image`.
Add a note explaining that the import path for color is image/color

https://codereview.appspot.com/118170043/diff/40001/content/methods/exercise-...
File content/methods/exercise-reader.go (right):

https://codereview.appspot.com/118170043/diff/40001/content/methods/exercise-...
content/methods/exercise-reader.go:5: type AReader struct{}
s/AReader/MyReader/

https://codereview.appspot.com/118170043/diff/40001/content/methods/exercise-...
File content/methods/exercise-stringer.go (right):

https://codereview.appspot.com/118170043/diff/40001/content/methods/exercise-...
content/methods/exercise-stringer.go:10: for n, a := range map[string]IPAddr{
I'd define a map variable so the for loops is easier to understand.

https://codereview.appspot.com/118170043/diff/40001/content/methods/stringer.go
File content/methods/stringer.go (right):

https://codereview.appspot.com/118170043/diff/40001/content/methods/stringer....
content/methods/stringer.go:11: return fmt.Sprintf("%v (%d years)", p.Name,
p.Age)
"%s (%d years)" or "%v (%v years)"

[adg, 2014-07-29 23:23:00]

PTAL

https://codereview.appspot.com/118170043/diff/40001/content/methods.article
File content/methods.article (right):

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:17: You can declare a method on _any_ named type in your
package, not just struct types.
On 2014/07/29 21:43:03, campoy wrote:
> I'm not sure about the "named" reference here. It might make people wonder
what
> that means.

Reworded.

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:64: * Stringers
On 2014/07/29 21:43:03, campoy wrote:
> Adding this section makes me think we should have at least a link to the
> printing format, or at least mention what %v does.

I think we can get away with leaving it out, for now.

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:66: One of the most ubiquitous interfaces is `Stringer`
defined by the `fmt` package.
On 2014/07/29 21:43:03, campoy wrote:
> Missing comma after `Stringer`?

Nope

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:66: One of the most ubiquitous interfaces is `Stringer`
defined by the `fmt` package.
On 2014/07/29 21:43:04, campoy wrote:
> Should Stringer be a link to its doc?

Done.

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:73: (and many others) look for this interface for
pretty-printing values.
On 2014/07/29 21:43:03, campoy wrote:
> I'd just say "to print" values. Pretty-printing is pretty subjective in this
> case.

Done.

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:97: pretty-printing values.)
On 2014/07/29 21:43:03, campoy wrote:
> /pretty-/d

Done.

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:105: }
On 2014/07/29 21:43:03, campoy wrote:
> add extra line using i
> 
> fmt.Printf("converted number: %v\n", i)

Done.

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:107: A nil error denotes success; a non-nil error
denotes failure.
On 2014/07/29 21:43:03, campoy wrote:
> `error`

Done.

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:136: which represents a stream of binary data.
On 2014/07/29 21:43:03, campoy wrote:
> a read-only stream of data.

Done.

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:138: The Go standard library contains
[[http://golang.org/search?q=Read#Global][many implementations]] of these
interfaces, from files to network connections to compressors to ciphers.
On 2014/07/29 21:43:04, campoy wrote:
> The from to construction with multiple "to"s reads awkward.
> 
> , including files, network connections, compressors, ciphers, and others.

Done.

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:161: * Exercise: rot13 Reader
On 2014/07/29 21:43:04, campoy wrote:
> ROT13?

What does ROT stand for? It's short for rotate. (I disagree with the wikipedia
editors about the capitalization)

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:165: For example, the
[[http://golang.org/pkg/compress/gzip/#NewReader][gzip.NewReader]] function
takes an `io.Reader` (a stream of gzipped data) and returns a `*gzip.Reader`
that also implements `io.Reader` (a stream of the decompressed data).
On 2014/07/29 21:43:03, campoy wrote:
> if you say gzipped, should you say gunzipped?

switched to "compressed"

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:229: *Note*: the `Rectangle` return value of the
`Bounds` method is actually an
[[http://golang.org/pkg/image/#Rectangle][`image.Rectangle`]], as the
declaration is inside package `image`.
On 2014/07/29 21:43:03, campoy wrote:
> Add a note explaining that the import path for color is image/color

See the next paragraph

https://codereview.appspot.com/118170043/diff/40001/content/methods/exercise-...
File content/methods/exercise-reader.go (right):

https://codereview.appspot.com/118170043/diff/40001/content/methods/exercise-...
content/methods/exercise-reader.go:5: type AReader struct{}
On 2014/07/29 21:43:04, campoy wrote:
> s/AReader/MyReader/

Done.

https://codereview.appspot.com/118170043/diff/40001/content/methods/exercise-...
File content/methods/exercise-stringer.go (right):

https://codereview.appspot.com/118170043/diff/40001/content/methods/exercise-...
content/methods/exercise-stringer.go:10: for n, a := range map[string]IPAddr{
On 2014/07/29 21:43:04, campoy wrote:
> I'd define a map variable so the for loops is easier to understand.

I thought it was cute to show people they can do this.
If you insist I'll revert it

https://codereview.appspot.com/118170043/diff/40001/content/methods/stringer.go
File content/methods/stringer.go (right):

https://codereview.appspot.com/118170043/diff/40001/content/methods/stringer....
content/methods/stringer.go:11: return fmt.Sprintf("%v (%d years)", p.Name,
p.Age)
On 2014/07/29 21:43:04, campoy wrote:
> "%s (%d years)" or "%v (%v years)"

Done.

[campoy, 2014-07-30 00:41:12]

https://codereview.appspot.com/118170043/diff/40001/content/methods.article
File content/methods.article (right):

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:161: * Exercise: rot13 Reader
On 2014/07/29 23:22:59, adg wrote:
> On 2014/07/29 21:43:04, campoy wrote:
> > ROT13?
> 
> What does ROT stand for? It's short for rotate. (I disagree with the wikipedia
> editors about the capitalization)

Then either rot13Reader (as you write later) or rot13 reader. The mixed
capitalization feels weird.

https://codereview.appspot.com/118170043/diff/40001/content/methods/exercise-...
File content/methods/exercise-stringer.go (right):

https://codereview.appspot.com/118170043/diff/40001/content/methods/exercise-...
content/methods/exercise-stringer.go:10: for n, a := range map[string]IPAddr{
On 2014/07/29 23:23:00, adg wrote:
> On 2014/07/29 21:43:04, campoy wrote:
> > I'd define a map variable so the for loops is easier to understand.
> 
> I thought it was cute to show people they can do this.
> If you insist I'll revert it

I insist, this looks confusing.

https://codereview.appspot.com/118170043/diff/60001/content/methods.article
File content/methods.article (right):

https://codereview.appspot.com/118170043/diff/60001/content/methods.article#n...
content/methods.article:108: A nil `error` denotes success; a non-nil error
denotes failure.
`error`

https://codereview.appspot.com/118170043/diff/60001/content/methods.article#n...
content/methods.article:137: which represents the read end of a stream of binary
data.
I'd drop 'binary', all data is somehow binary anyway.

[adg, 2014-07-30 00:47:19]

PTAL

https://codereview.appspot.com/118170043/diff/40001/content/methods.article
File content/methods.article (right):

https://codereview.appspot.com/118170043/diff/40001/content/methods.article#n...
content/methods.article:161: * Exercise: rot13 Reader
On 2014/07/30 00:41:11, campoy wrote:
> On 2014/07/29 23:22:59, adg wrote:
> > On 2014/07/29 21:43:04, campoy wrote:
> > > ROT13?
> > 
> > What does ROT stand for? It's short for rotate. (I disagree with the
wikipedia
> > editors about the capitalization)
> 
> Then either rot13Reader (as you write later) or rot13 reader. The mixed
> capitalization feels weird.

Done.

https://codereview.appspot.com/118170043/diff/40001/content/methods/exercise-...
File content/methods/exercise-stringer.go (right):

https://codereview.appspot.com/118170043/diff/40001/content/methods/exercise-...
content/methods/exercise-stringer.go:10: for n, a := range map[string]IPAddr{
On 2014/07/30 00:41:11, campoy wrote:
> On 2014/07/29 23:23:00, adg wrote:
> > On 2014/07/29 21:43:04, campoy wrote:
> > > I'd define a map variable so the for loops is easier to understand.
> > 
> > I thought it was cute to show people they can do this.
> > If you insist I'll revert it
> 
> I insist, this looks confusing.

Done.

https://codereview.appspot.com/118170043/diff/60001/content/methods.article
File content/methods.article (right):

https://codereview.appspot.com/118170043/diff/60001/content/methods.article#n...
content/methods.article:108: A nil `error` denotes success; a non-nil error
denotes failure.
On 2014/07/30 00:41:11, campoy wrote:
> `error`

Done.

https://codereview.appspot.com/118170043/diff/60001/content/methods.article#n...
content/methods.article:137: which represents the read end of a stream of binary
data.
On 2014/07/30 00:41:11, campoy wrote:
> I'd drop 'binary', all data is somehow binary anyway.

Done.

[campoy, 2014-07-30 00:48:14]

LGTM


On Tue, Jul 29, 2014 at 5:47 PM, <adg@golang.org> wrote:

> PTAL
>
>
>
> https://codereview.appspot.com/118170043/diff/40001/
> content/methods.article
> File content/methods.article (right):
>
> https://codereview.appspot.com/118170043/diff/40001/
> content/methods.article#newcode161
> content/methods.article:161: * Exercise: rot13 Reader
> On 2014/07/30 00:41:11, campoy wrote:
>
>> On 2014/07/29 23:22:59, adg wrote:
>> > On 2014/07/29 21:43:04, campoy wrote:
>> > > ROT13?
>> >
>> > What does ROT stand for? It's short for rotate. (I disagree with the
>>
> wikipedia
>
>> > editors about the capitalization)
>>
>
>  Then either rot13Reader (as you write later) or rot13 reader. The
>>
> mixed
>
>> capitalization feels weird.
>>
>
> Done.
>
>
> https://codereview.appspot.com/118170043/diff/40001/
> content/methods/exercise-stringer.go
> File content/methods/exercise-stringer.go (right):
>
> https://codereview.appspot.com/118170043/diff/40001/
> content/methods/exercise-stringer.go#newcode10
> content/methods/exercise-stringer.go:10: for n, a := range
> map[string]IPAddr{
> On 2014/07/30 00:41:11, campoy wrote:
>
>> On 2014/07/29 23:23:00, adg wrote:
>> > On 2014/07/29 21:43:04, campoy wrote:
>> > > I'd define a map variable so the for loops is easier to
>>
> understand.
>
>> >
>> > I thought it was cute to show people they can do this.
>> > If you insist I'll revert it
>>
>
>  I insist, this looks confusing.
>>
>
> Done.
>
>
> https://codereview.appspot.com/118170043/diff/60001/
> content/methods.article
> File content/methods.article (right):
>
> https://codereview.appspot.com/118170043/diff/60001/
> content/methods.article#newcode108
> content/methods.article:108: A nil `error` denotes success; a non-nil
> error denotes failure.
> On 2014/07/30 00:41:11, campoy wrote:
>
>> `error`
>>
>
> Done.
>
>
> https://codereview.appspot.com/118170043/diff/60001/
> content/methods.article#newcode137
> content/methods.article:137: which represents the read end of a stream
> of binary data.
> On 2014/07/30 00:41:11, campoy wrote:
>
>> I'd drop 'binary', all data is somehow binary anyway.
>>
>
> Done.
>
> https://codereview.appspot.com/118170043/
>

[adg, 2014-07-30 00:50:59]

*** Submitted as https://code.google.com/p/go-tour/source/detail?r=7ecc21f5dc9a
***

go-tour: add Stringer, Reader discussions and exercises; other tweaks

LGTM=campoy
R=campoy
CC=golang-codereviews
https://codereview.appspot.com/118170043