Issue 47870043: code review 47870043: encoding/xml: add test for EncodeToken
Descriptionencoding/xml: add test for EncodeToken
Patch Set 1 #Patch Set 2 : diff -r 76ba983c4f3d https://code.google.com/p/go #Patch Set 3 : diff -r 76ba983c4f3d https://code.google.com/p/go #Patch Set 4 : diff -r 76ba983c4f3d https://code.google.com/p/go #Patch Set 5 : diff -r 76ba983c4f3d https://code.google.com/p/go #
Total comments: 4
Patch Set 6 : diff -r e5b12367190b https://code.google.com/p/go #Patch Set 7 : diff -r e5b12367190b https://code.google.com/p/go #Patch Set 8 : diff -r e5b12367190b https://code.google.com/p/go #Patch Set 9 : diff -r e5b12367190b https://code.google.com/p/go #Patch Set 10 : diff -r aaeb25be6d7f https://code.google.com/p/go #MessagesTotal messages: 16
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.
You might want to add "Updates issue 6094." to the description. -josh https://codereview.appspot.com/47870043/diff/80001/src/pkg/encoding/xml/marsh... File src/pkg/encoding/xml/marshal_test.go (right): https://codereview.appspot.com/47870043/diff/80001/src/pkg/encoding/xml/marsh... src/pkg/encoding/xml/marshal_test.go:1154: t Token Nit: maybe tok instead of t, to avoid easy confusion with the testing.T.
Sign in to reply to this message.
Sorry for the delay. If you could address the review points and remail please. When updating the issue description the form is Update issue NNNN Commentary There should be no period following 'Update issue NNNN' and 'Update issue NNNN' should come at the top of the description so the remainder of the commentary is appended to the issue. https://codereview.appspot.com/47870043/diff/80001/src/pkg/encoding/xml/marsh... File src/pkg/encoding/xml/marshal_test.go (right): https://codereview.appspot.com/47870043/diff/80001/src/pkg/encoding/xml/marsh... src/pkg/encoding/xml/marshal_test.go:1154: t Token On 2014/01/06 02:57:23, josharian wrote: > Nit: maybe tok instead of t, to avoid easy confusion with the testing.T. Yeah, can't hurt, there are a lot of ts in that function.
Sign in to reply to this message.
https://codereview.appspot.com/47870043/diff/80001/src/pkg/encoding/xml/marsh... File src/pkg/encoding/xml/marshal_test.go (right): https://codereview.appspot.com/47870043/diff/80001/src/pkg/encoding/xml/marsh... src/pkg/encoding/xml/marshal_test.go:1154: t Token On 2014/01/06 02:57:23, josharian wrote: > Nit: maybe tok instead of t, to avoid easy confusion with the testing.T. Done. https://codereview.appspot.com/47870043/diff/80001/src/pkg/encoding/xml/marsh... src/pkg/encoding/xml/marshal_test.go:1154: t Token On 2014/01/31 03:06:42, dfc wrote: > On 2014/01/06 02:57:23, josharian wrote: > > Nit: maybe tok instead of t, to avoid easy confusion with the testing.T. > > Yeah, can't hurt, there are a lot of ts in that function. Done.
Sign in to reply to this message.
On 2014/01/31 03:06:42, dfc wrote: > Sorry for the delay. > > If you could address the review points and remail please. > > When updating the issue description the form is > > Update issue NNNN > > Commentary > > There should be no period following 'Update issue NNNN' and 'Update issue NNNN' > should come at the top of the description so the remainder of the commentary is > appended to the issue. > > https://codereview.appspot.com/47870043/diff/80001/src/pkg/encoding/xml/marsh... > File src/pkg/encoding/xml/marshal_test.go (right): > > https://codereview.appspot.com/47870043/diff/80001/src/pkg/encoding/xml/marsh... > src/pkg/encoding/xml/marshal_test.go:1154: t Token > On 2014/01/06 02:57:23, josharian wrote: > > Nit: maybe tok instead of t, to avoid easy confusion with the testing.T. > > Yeah, can't hurt, there are a lot of ts in that function. About the "Update issue NNNN" part, does it look like this? Description: Update issue 6094 encoding/xml: add test for EncodeToken I'm getting a warning about the description ("Your CL description appears not to use the standard form.") so I'm wondering if I should ignore it.
Sign in to reply to this message.
On 2014/02/01 11:47:01, shawnps wrote: > On 2014/01/31 03:06:42, dfc wrote: > > Sorry for the delay. > > > > If you could address the review points and remail please. > > > > When updating the issue description the form is > > > > Update issue NNNN > > > > Commentary > > > > There should be no period following 'Update issue NNNN' and 'Update issue > NNNN' > > should come at the top of the description so the remainder of the commentary > is > > appended to the issue. > > > > > https://codereview.appspot.com/47870043/diff/80001/src/pkg/encoding/xml/marsh... > > File src/pkg/encoding/xml/marshal_test.go (right): > > > > > https://codereview.appspot.com/47870043/diff/80001/src/pkg/encoding/xml/marsh... > > src/pkg/encoding/xml/marshal_test.go:1154: t Token > > On 2014/01/06 02:57:23, josharian wrote: > > > Nit: maybe tok instead of t, to avoid easy confusion with the testing.T. > > > > Yeah, can't hurt, there are a lot of ts in that function. > > About the "Update issue NNNN" part, does it look like this? > > Description: > Update issue 6094 > > encoding/xml: add test for EncodeToken > > I'm getting a warning about the description ("Your CL description appears not to > use the standard form.") so I'm wondering if I should ignore it. The encoding/xml line should still be the first line. The "Update issue" should be below it. Ian
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, josharian@gmail.com, dave@cheney.net, iant@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2014/02/01 17:42:58, iant wrote: > On 2014/02/01 11:47:01, shawnps wrote: > > On 2014/01/31 03:06:42, dfc wrote: > > > Sorry for the delay. > > > > > > If you could address the review points and remail please. > > > > > > When updating the issue description the form is > > > > > > Update issue NNNN > > > > > > Commentary > > > > > > There should be no period following 'Update issue NNNN' and 'Update issue > > NNNN' > > > should come at the top of the description so the remainder of the commentary > > is > > > appended to the issue. > > > > > > > > > https://codereview.appspot.com/47870043/diff/80001/src/pkg/encoding/xml/marsh... > > > File src/pkg/encoding/xml/marshal_test.go (right): > > > > > > > > > https://codereview.appspot.com/47870043/diff/80001/src/pkg/encoding/xml/marsh... > > > src/pkg/encoding/xml/marshal_test.go:1154: t Token > > > On 2014/01/06 02:57:23, josharian wrote: > > > > Nit: maybe tok instead of t, to avoid easy confusion with the testing.T. > > > > > > Yeah, can't hurt, there are a lot of ts in that function. > > > > About the "Update issue NNNN" part, does it look like this? > > > > Description: > > Update issue 6094 > > > > encoding/xml: add test for EncodeToken > > > > I'm getting a warning about the description ("Your CL description appears not > to > > use the standard form.") so I'm wondering if I should ignore it. > > The encoding/xml line should still be the first line. The "Update issue" should > be below it. > > Ian All right, thanks. I put the "Update issue" line below encoding/xml line, but I'm not sure what is supposed to happen so I can't confirm that it worked.
Sign in to reply to this message.
> All right, thanks. I put the "Update issue" line below encoding/xml line, but > I'm not sure what is supposed to happen so I can't confirm that it worked. Whenever you update the description via hg change, you need to do hg upload for it to be visible to others. hg mail does the same, but obviously sends mail.
Sign in to reply to this message.
On 2014/02/05 02:46:05, dfc wrote: > > All right, thanks. I put the "Update issue" line below encoding/xml line, but > > I'm not sure what is supposed to happen so I can't confirm that it worked. > > Whenever you update the description via hg change, you need to do hg upload for > it to be visible to others. hg mail does the same, but obviously sends mail. I've done that, is there something that should happen? When I run "hg change 47870043" after having already edited the description and uploaded, it looks like this: # Change list. # Lines beginning with # are ignored. # Multi-line values should be indented. Author: shawn.p.smith@gmail.com URL: https://codereview.appspot.com/47870043 # cannot edit Reviewer: golang-codereviews@googlegroups.com, josharian@gmail.com, dave@cheney. net, iant@golang.org CC: golang-codereviews@googlegroups.com Description: encoding/xml: add test for EncodeToken Update issue 6094 Files: src/pkg/encoding/xml/marshal_test.go
Sign in to reply to this message.
On Thu, Feb 6, 2014 at 4:23 AM, <shawn.p.smith@gmail.com> wrote: > On 2014/02/05 02:46:05, dfc wrote: >> >> > All right, thanks. I put the "Update issue" line below encoding/xml > > line, but >> >> > I'm not sure what is supposed to happen so I can't confirm that it > > worked. > >> Whenever you update the description via hg change, you need to do hg > > upload for >> >> it to be visible to others. hg mail does the same, but obviously sends > > mail. > > I've done that, is there something that should happen? > > When I run "hg change 47870043" after having already edited the > description and uploaded, it looks like this: > > # Change list. > # Lines beginning with # are ignored. > # Multi-line values should be indented. > > Author: shawn.p.smith@gmail.com > URL: https://codereview.appspot.com/47870043 # cannot edit > > Reviewer: golang-codereviews@googlegroups.com, josharian@gmail.com, > dave@cheney. net, iant@golang.org > CC: golang-codereviews@googlegroups.com > > Description: > > encoding/xml: add test for EncodeToken > Update issue 6094 I think Brad was also saying that there was a case where he could not edit the description. I don't know what is going on. I have not seen this myself. Normally when you run "hg change", edit the description, and then run "hg upload", it changes on the codereview site. You could try going to https://codereview.appspot.com/47870043 and editing the description directly by clicking on "Edit Issue". Ian
Sign in to reply to this message.
I think it was a rietveld server-side problem because when it happened, I also couldn't edit the issue directly on the website and see it take effect there. And then I think codereview.py updates the local state when you run hg pending or hg submit, so if the server returns the wrong stuff (over-aggressive caching? but I saw no recent suspicious rietveld code changes), then it looks like it's a codereview.py problem. Maybe. On Thu, Feb 6, 2014 at 8:47 AM, Ian Lance Taylor <iant@golang.org> wrote: > On Thu, Feb 6, 2014 at 4:23 AM, <shawn.p.smith@gmail.com> wrote: > > On 2014/02/05 02:46:05, dfc wrote: > >> > >> > All right, thanks. I put the "Update issue" line below encoding/xml > > > > line, but > >> > >> > I'm not sure what is supposed to happen so I can't confirm that it > > > > worked. > > > >> Whenever you update the description via hg change, you need to do hg > > > > upload for > >> > >> it to be visible to others. hg mail does the same, but obviously sends > > > > mail. > > > > I've done that, is there something that should happen? > > > > When I run "hg change 47870043" after having already edited the > > description and uploaded, it looks like this: > > > > # Change list. > > # Lines beginning with # are ignored. > > # Multi-line values should be indented. > > > > Author: shawn.p.smith@gmail.com > > URL: https://codereview.appspot.com/47870043 # cannot edit > > > > Reviewer: golang-codereviews@googlegroups.com, josharian@gmail.com, > > dave@cheney. net, iant@golang.org > > CC: golang-codereviews@googlegroups.com > > > > Description: > > > > encoding/xml: add test for EncodeToken > > Update issue 6094 > > I think Brad was also saying that there was a case where he could not > edit the description. I don't know what is going on. I have not seen > this myself. Normally when you run "hg change", edit the description, > and then run "hg upload", it changes on the codereview site. > > You could try going to https://codereview.appspot.com/47870043 and > editing the description directly by clicking on "Edit Issue". > > Ian > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. >
Sign in to reply to this message.
ping. Shawn, do you want to hg mail this CL again and we'll try to get it moving again.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, josharian@gmail.com, dave@cheney.net, iant@golang.org, bradfitz@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=3091cbc3b91c *** encoding/xml: add test for EncodeToken LGTM=rsc R=golang-codereviews, josharian, dave, iant, bradfitz, rsc CC=golang-codereviews https://codereview.appspot.com/47870043 Committer: Russ Cox <rsc@golang.org>
Sign in to reply to this message.
|
