|
|
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
please change the first line of the description to: encoding/json: add example for Indent, clarify the docs. or something like that. no need for spaces before the first line.
https://codereview.appspot.com/97840044/diff/40001/src/pkg/encoding/json/exam... File src/pkg/encoding/json/example_test.go (right): https://codereview.appspot.com/97840044/diff/40001/src/pkg/encoding/json/exam... src/pkg/encoding/json/example_test.go:148: out.Write([]byte("ʕ◔π◔ʔ{\nʕ◔π◔ʔ\t\"destinations\": ")) We think this is too cute. Remove the emojicons. Just write XYX or ":" or "=" or something else. https://codereview.appspot.com/97840044/diff/40001/src/pkg/encoding/json/exam... src/pkg/encoding/json/example_test.go:149: // n.b. the previous write did not end with newline, and the following does not start with the "ʕ◔π◔ʔ" Drop the "n.b.". We avoid ie, e.g., etc in docs.
> please change the first line of the description to: > encoding/json: add example for Indent, clarify the docs. > > or something like that. > > no need for spaces before the first line. Apart from what minux said, please run fmt(1) on the description.
On 2014年04月30日 06:29:05, minux wrote: > please change the first line of the description to: > encoding/json: add example for Indent, clarify the docs. Thanks for the suggested phrasing. > no need for spaces before the first line. There was funkiness with my vim config. Looks better above to me; please let me know if I still didn't get it right.
On 2014年04月30日 14:29:22, bradfitz wrote: > We think this is too cute. Remove the emojicons. Just write XYX or ":" or "=" > or something else. Addressed. I went with 'Δ'; if you still think that's too fancy, I'll change it to something ascii. > Drop the "n.b.". We avoid ie, e.g., etc in docs. That makes sense. Incidentally I wasn't sure what to do with wrapping, so I matched what was going on elsewhere in the docs (seems to wrap 75-ish)
On 2014年04月30日 14:32:03, aram wrote: > Apart from what minux said, please run fmt(1) on the description. I ran with the fmt(1) defaults of --width=75; look better?
On 2014年05月01日 04:51:20, smcquay wrote: > On 2014年04月30日 14:32:03, aram wrote: > > Apart from what minux said, please run fmt(1) on the description. > > I ran with the fmt(1) defaults of --width=75; look better? The fmt(1) may just have been intended for the CL description (just move the "to" to the next line) For the Go code itself, I don't believe there are any hard-and-fast limits. Wrap if it feels too long to you. As Effective Go likes to say, don't worry about overflowing a punched card. :)
Yes, CL description, there are no hard limits for Go code. If it feels right, it's not too long. Also we never want to reformat existing code just to wrap it.
Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com, bradfitz@golang.org, aram@mgk.ro, robert.hencke@gmail.com (cc: golang-codereviews@googlegroups.com), Please take another look.
LGTM - my comments here are just alternatives, not requests. https://codereview.appspot.com/97840044/diff/160001/src/pkg/encoding/json/exa... File src/pkg/encoding/json/example_test.go (right): https://codereview.appspot.com/97840044/diff/160001/src/pkg/encoding/json/exa... src/pkg/encoding/json/example_test.go:149: // the previous write did not end with newline, and the following does Perhaps this? // The previous Write did not end with a new line, so Indent waits // for the next new line before beginning to indent with "Δ\t". https://codereview.appspot.com/97840044/diff/160001/src/pkg/encoding/json/exa... src/pkg/encoding/json/example_test.go:156: // Δ{ Windows struggled with Unicode console output last I checked, so godoc -ex=true might display interesting things here here. As a Windows user, I am used to the Windows console being a constant disappointment, though. Just noting for thought.
https://codereview.appspot.com/97840044/diff/160001/src/pkg/encoding/json/exa... File src/pkg/encoding/json/example_test.go (right): https://codereview.appspot.com/97840044/diff/160001/src/pkg/encoding/json/exa... src/pkg/encoding/json/example_test.go:149: // the previous write did not end with newline, and the following does The delta character has no explanatory value here. I find it confusing. Please replace it.
Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com, bradfitz@golang.org, aram@mgk.ro, robert.hencke@gmail.com, r@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Addressed requests. https://codereview.appspot.com/97840044/diff/40001/src/pkg/encoding/json/exam... File src/pkg/encoding/json/example_test.go (right): https://codereview.appspot.com/97840044/diff/40001/src/pkg/encoding/json/exam... src/pkg/encoding/json/example_test.go:148: out.Write([]byte("ʕ◔π◔ʔ{\nʕ◔π◔ʔ\t\"destinations\": ")) On 2014年04月30日 14:29:22, bradfitz wrote: > We think this is too cute. Remove the emojicons. Just write XYX or ":" or "=" > or something else. Done. https://codereview.appspot.com/97840044/diff/40001/src/pkg/encoding/json/exam... src/pkg/encoding/json/example_test.go:149: // n.b. the previous write did not end with newline, and the following does not start with the "ʕ◔π◔ʔ" On 2014年04月30日 14:29:22, bradfitz wrote: > Drop the "n.b.". We avoid ie, e.g., etc in docs. Done. https://codereview.appspot.com/97840044/diff/160001/src/pkg/encoding/json/exa... File src/pkg/encoding/json/example_test.go (right): https://codereview.appspot.com/97840044/diff/160001/src/pkg/encoding/json/exa... src/pkg/encoding/json/example_test.go:149: // the previous write did not end with newline, and the following does On 2014年05月02日 14:41:37, r wrote: > The delta character has no explanatory value here. I find it confusing. Please > replace it. Done.
LGTM ʕ◔π◔ʔ
https://codereview.appspot.com/97840044/diff/200001/src/pkg/encoding/json/exa... File src/pkg/encoding/json/example_test.go (right): https://codereview.appspot.com/97840044/diff/200001/src/pkg/encoding/json/exa... src/pkg/encoding/json/example_test.go:155: // Output: Wouldn't it be clearer to write an example that only demonstrates the behavior of Indent? Just glancing at this I would assume Indent does the whole job and no further work is required. https://codereview.appspot.com/97840044/diff/200001/src/pkg/encoding/json/ind... File src/pkg/encoding/json/indent.go (right): https://codereview.appspot.com/97840044/diff/200001/src/pkg/encoding/json/ind... src/pkg/encoding/json/indent.go:69: // Each element following the first in a JSON object or array begins this is wrong Each **element** in a JSON **object** or **array** begins on a new, indented line. That doesn't mean that the opening bracket or brace of an object or array begins on a new, indented line. please revert this https://codereview.appspot.com/97840044/diff/200001/src/pkg/encoding/json/ind... src/pkg/encoding/json/indent.go:71: // more copies of indent according to the indentation nesting. The revert (including the wrapping of "The") https://codereview.appspot.com/97840044/diff/200001/src/pkg/encoding/json/ind... src/pkg/encoding/json/indent.go:72: // data appended to dst does not begin with the prefix nor any this change is OK
https://codereview.appspot.com/97840044/diff/200001/src/pkg/encoding/json/exa... File src/pkg/encoding/json/example_test.go (right): https://codereview.appspot.com/97840044/diff/200001/src/pkg/encoding/json/exa... src/pkg/encoding/json/example_test.go:155: // Output: On 2014年05月05日 01:53:31, adg wrote: > Wouldn't it be clearer to write an example that only demonstrates the behavior > of Indent? The behavior of Indent wasn't clear to me until I considered it in this context; that's why I had the Indent nestled between other bits of json. If there isn't much value having it used in context like this (perhaps the example hides edge behavior?) I can revert it to be a simple example of the use of the function. Thoughts? > Just glancing at this I would assume Indent does the whole job and no further > work is required. My hope was that the out.Write calls above would have given sufficient context. Perhaps they don't? https://codereview.appspot.com/97840044/diff/200001/src/pkg/encoding/json/ind... File src/pkg/encoding/json/indent.go (right): https://codereview.appspot.com/97840044/diff/200001/src/pkg/encoding/json/ind... src/pkg/encoding/json/indent.go:69: // Each element following the first in a JSON object or array begins On 2014年05月05日 01:53:31, adg wrote: > this is wrong > Each **element** in a JSON **object** or **array** begins on a new, indented > line. > > That doesn't mean that the opening bracket or brace of an object or array begins > on a new, indented line. > > please revert this Done. https://codereview.appspot.com/97840044/diff/200001/src/pkg/encoding/json/ind... src/pkg/encoding/json/indent.go:71: // more copies of indent according to the indentation nesting. The On 2014年05月05日 01:53:31, adg wrote: > revert (including the wrapping of "The") Done. https://codereview.appspot.com/97840044/diff/200001/src/pkg/encoding/json/ind... src/pkg/encoding/json/indent.go:72: // data appended to dst does not begin with the prefix nor any On 2014年05月05日 01:53:31, adg wrote: > this change is OK Done.
https://codereview.appspot.com/97840044/diff/200001/src/pkg/encoding/json/exa... File src/pkg/encoding/json/example_test.go (right): https://codereview.appspot.com/97840044/diff/200001/src/pkg/encoding/json/exa... src/pkg/encoding/json/example_test.go:155: // Output: On 2014年05月07日 03:41:34, smcquay wrote: > On 2014年05月05日 01:53:31, adg wrote: > > Wouldn't it be clearer to write an example that only demonstrates the behavior > > of Indent? > > The behavior of Indent wasn't clear to me until I considered it in this context; > that's why I had the Indent nestled between other bits of json. > > If there isn't much value having it used in context like this (perhaps the > example hides edge behavior?) I can revert it to be a simple example of the use > of the function. > > Thoughts? I'd like to take a look at a stripped back example. Sorry for being picky about this, I just think seeing the function's behavior on its own might be more illustrative. > > Just glancing at this I would assume Indent does the whole job and no further > > work is required. > > My hope was that the out.Write calls above would have given sufficient context. > Perhaps they don't? Yeah, but on a brief reading you might not notice them.
Addressing adg's review. https://codereview.appspot.com/97840044/diff/200001/src/pkg/encoding/json/exa... File src/pkg/encoding/json/example_test.go (right): https://codereview.appspot.com/97840044/diff/200001/src/pkg/encoding/json/exa... src/pkg/encoding/json/example_test.go:155: // Output: On 2014年05月08日 04:00:50, adg wrote: > On 2014年05月07日 03:41:34, smcquay wrote: > > On 2014年05月05日 01:53:31, adg wrote: > > > Wouldn't it be clearer to write an example that only demonstrates the > behavior > > > of Indent? > > > > The behavior of Indent wasn't clear to me until I considered it in this > context; > > that's why I had the Indent nestled between other bits of json. > > > > If there isn't much value having it used in context like this (perhaps the > > example hides edge behavior?) I can revert it to be a simple example of the > use > > of the function. > > > > Thoughts? > > I'd like to take a look at a stripped back example. Sorry for being picky about > this, I just think seeing the function's behavior on its own might be more > illustrative. > > > > Just glancing at this I would assume Indent does the whole job and no > further > > > work is required. > > > > My hope was that the out.Write calls above would have given sufficient > context. > > Perhaps they don't? > > Yeah, but on a brief reading you might not notice them. Done.
Yeah that's much clearer to my eyes. https://codereview.appspot.com/97840044/diff/240001/src/pkg/encoding/json/exa... File src/pkg/encoding/json/example_test.go (right): https://codereview.appspot.com/97840044/diff/240001/src/pkg/encoding/json/exa... src/pkg/encoding/json/example_test.go:138: Road{"Diamond Fork", 29}, you can drop the "Road" on each of these lines
Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com, bradfitz@golang.org, aram@mgk.ro, robert.hencke@gmail.com, r@golang.org, adg@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
*Actually* addressing adg's review. https://codereview.appspot.com/97840044/diff/240001/src/pkg/encoding/json/exa... File src/pkg/encoding/json/example_test.go (right): https://codereview.appspot.com/97840044/diff/240001/src/pkg/encoding/json/exa... src/pkg/encoding/json/example_test.go:138: Road{"Diamond Fork", 29}, On 2014年05月08日 06:26:52, adg wrote: > you can drop the "Road" on each of these lines Done.
LGTM Thanks for sticking with it!
*** Submitted as https://code.google.com/p/go/source/detail?r=f7e221a34ec6 *** encoding/json: add example for Indent, clarify the docs. There was confusion in the behavior of json.Indent; This change attempts to clarify the behavior by providing a bit more verbiage to the documentation as well as provide an example function. Fixes issue 7821. LGTM=robert.hencke, adg R=golang-codereviews, minux.ma, bradfitz, aram, robert.hencke, r, adg CC=golang-codereviews https://codereview.appspot.com/97840044 Committer: Andrew Gerrand <adg@golang.org>