Issue 97840044: code review 97840044: encoding/json: add example for Indent, clarify the docs.
Descriptionencoding/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.
Patch Set 1 #Patch Set 2 : diff -r 824f981dd4b7 https://code.google.com/p/go/ #Patch Set 3 : diff -r 824f981dd4b7 https://code.google.com/p/go/ #
Total comments: 4
Patch Set 4 : diff -r c19d7fd53785 https://code.google.com/p/go/ #Patch Set 5 : diff -r c19d7fd53785 https://code.google.com/p/go/ #Patch Set 6 : diff -r c19d7fd53785 https://code.google.com/p/go/ #Patch Set 7 : diff -r c19d7fd53785 https://code.google.com/p/go/ #Patch Set 8 : diff -r b0443478e712 https://code.google.com/p/go/ #Patch Set 9 : diff -r b0443478e712 https://code.google.com/p/go/ #
Total comments: 4
Patch Set 10 : diff -r 2e591e82a8c8 https://code.google.com/p/go/ #Patch Set 11 : diff -r 2e591e82a8c8 https://code.google.com/p/go/ #
Total comments: 10
Patch Set 12 : diff -r 2a7aa0e6641d https://code.google.com/p/go/ #Patch Set 13 : diff -r e3d533e55750 https://code.google.com/p/go/ #
Total comments: 2
Patch Set 14 : diff -r e3d533e55750 https://code.google.com/p/go/ #Patch Set 15 : diff -r e3d533e55750 https://code.google.com/p/go/ #
MessagesTotal messages: 24
Hello 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.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
> 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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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)
Sign in to reply to this message.
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?
Sign in to reply to this message.
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. :)
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
LGTM ʕ◔ϖ◔ʔ
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
*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.
Sign in to reply to this message.
LGTM Thanks for sticking with it!
Sign in to reply to this message.
*** 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>
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
