Issue 4906043: code review 4906043: exp/template/html: defines a parse context for a subset... (Closed)
|
Created:
13 years, 8 months ago by MikeSamuel Modified:
13 years, 8 months ago Reviewers:
CC:
rsc, r, nigeltao, r2, golang-dev Visibility:
Public. |
Descriptionexp/template/html: defines a parse context for a subset of HTML.
This defines just enough context to distinguish HTML URI attributes
from parsed character data.
It does not affect any public module API as I thought I would get
early comment on style for defining enumerations and tables.
Patch Set 1 #Patch Set 2 : code review 4906043: exp/template/html: defines a parse context for a subset... #Patch Set 3 : diff -r c94708477811 https://go.googlecode.com/hg/ #Patch Set 4 : diff -r c94708477811 https://go.googlecode.com/hg/ #
Total comments: 10
Patch Set 5 : diff -r c94708477811 https://go.googlecode.com/hg/ #
Total comments: 8
Patch Set 6 : diff -r c94708477811 https://go.googlecode.com/hg/ #
Total comments: 6
Patch Set 7 : diff -r a8d309fd526f https://go.googlecode.com/hg/ #Patch Set 8 : diff -r a8d309fd526f https://go.googlecode.com/hg/ #
Total comments: 4
Patch Set 9 : diff -r 1e3e5c27e0a3 https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 10 : diff -r e7bbaa4eae1f https://go.googlecode.com/hg/ #Patch Set 11 : diff -r daa11382c852 https://go.googlecode.com/hg/ #MessagesTotal messages: 16
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
http://codereview.appspot.com/4906043/diff/7001/src/pkg/exp/template/html/con... File src/pkg/exp/template/html/context.go (right): http://codereview.appspot.com/4906043/diff/7001/src/pkg/exp/template/html/con... src/pkg/exp/template/html/context.go:22: // GoString obeys the contract of fmt.GoStringer. It's not clear why you want to implement GoString. Usually you just implement String. It's rare to use %#v (which is what needs GoString) instead of %v. http://codereview.appspot.com/4906043/diff/7001/src/pkg/exp/template/html/con... src/pkg/exp/template/html/context.go:24: return "context{state: " + c.state.GoString() + use fmt.Sprintf http://codereview.appspot.com/4906043/diff/7001/src/pkg/exp/template/html/con... src/pkg/exp/template/html/context.go:44: stateHtmlTag state = iota drop state = iota on this and future lines when there's no type = val, it gets inherited from the previous line. http://codereview.appspot.com/4906043/diff/7001/src/pkg/exp/template/html/con... src/pkg/exp/template/html/context.go:51: // numStates is the count of states you don't use this, and probably don't need it. http://codereview.appspot.com/4906043/diff/7001/src/pkg/exp/template/html/con... src/pkg/exp/template/html/context.go:66: panic("Illegal state " + strconv.Uitoa(uint(s))) It's usually better for a string method to return something useful instead of panicking. Then if it happens during an error print (and it always does) you can get the rest of the error message too. return fmt.Sprintf("state(%d)", int(s))
Sign in to reply to this message.
Incremental diffs at http://codereview.appspot.com/4906043/diff2/7001:2003/src/pkg/exp/template/ht... http://codereview.appspot.com/4906043/diff/7001/src/pkg/exp/template/html/con... File src/pkg/exp/template/html/context.go (right): http://codereview.appspot.com/4906043/diff/7001/src/pkg/exp/template/html/con... src/pkg/exp/template/html/context.go:22: // GoString obeys the contract of fmt.GoStringer. On 2011/08/16 21:58:24, rsc wrote: > It's not clear why you want to implement GoString. > Usually you just implement String. It's rare to > use %#v (which is what needs GoString) instead of %v. GoString() -> String() in 3 definitions and 2 uses. http://codereview.appspot.com/4906043/diff/7001/src/pkg/exp/template/html/con... src/pkg/exp/template/html/context.go:24: return "context{state: " + c.state.GoString() + On 2011/08/16 21:58:24, rsc wrote: > use fmt.Sprintf Done, and replaced 2 uses of strconv.Uitoa below with fmt.Sprintf to minimize imports. http://codereview.appspot.com/4906043/diff/7001/src/pkg/exp/template/html/con... src/pkg/exp/template/html/context.go:44: stateHtmlTag state = iota On 2011/08/16 21:58:24, rsc wrote: > drop state = iota on this and future lines > when there's no type = val, it gets inherited > from the previous line. Done for 2 const statements. http://codereview.appspot.com/4906043/diff/7001/src/pkg/exp/template/html/con... src/pkg/exp/template/html/context.go:51: // numStates is the count of states On 2011/08/16 21:58:24, rsc wrote: > you don't use this, and probably don't need it. Done in 2 places. http://codereview.appspot.com/4906043/diff/7001/src/pkg/exp/template/html/con... src/pkg/exp/template/html/context.go:66: panic("Illegal state " + strconv.Uitoa(uint(s))) On 2011/08/16 21:58:24, rsc wrote: > It's usually better for a string method to return something useful > instead of panicking. Then if it happens during an error print > (and it always does) you can get the rest of the error message too. > > return fmt.Sprintf("state(%d)", int(s)) Done in 2 places.
Sign in to reply to this message.
lgtm leaving for r http://codereview.appspot.com/4906043/diff/2003/src/pkg/exp/template/html/con... File src/pkg/exp/template/html/context.go (right): http://codereview.appspot.com/4906043/diff/2003/src/pkg/exp/template/html/con... src/pkg/exp/template/html/context.go:18: state If you are trying to inherit the methods of state, then leave this as is. If you are using this as a shorthand for writing 'state state', that's typically written 'state state'. Same for delimType. Actually for the latter I would write delim delimType. http://codereview.appspot.com/4906043/diff/2003/src/pkg/exp/template/html/con... src/pkg/exp/template/html/context.go:22: // GoString obeys the contract of fmt.Stringer. can delete this. String is so common that we don't bother commenting it. http://codereview.appspot.com/4906043/diff/2003/src/pkg/exp/template/html/con... src/pkg/exp/template/html/context.go:53: switch s { This is about at the limit where you'd want to use a switch. If you think the state set will grow, better to use a table. For example http://golang.org/src/pkg/debug/dwarf/const.go#L164 http://codereview.appspot.com/4906043/diff/2003/src/pkg/exp/template/html/con... src/pkg/exp/template/html/context.go:63: return fmt.Sprintf("Illegal state %d", uint(s)) usually lower case, because these get stuck into the middle of other sentences. s/I/i/
Sign in to reply to this message.
Incremental diffs at http://codereview.appspot.com/4906043/diff2/2003:11003/src/pkg/exp/template/h... http://codereview.appspot.com/4906043/diff/2003/src/pkg/exp/template/html/con... File src/pkg/exp/template/html/context.go (right): http://codereview.appspot.com/4906043/diff/2003/src/pkg/exp/template/html/con... src/pkg/exp/template/html/context.go:18: state On 2011/08/16 22:13:33, rsc wrote: > If you are trying to inherit the methods of state, > then leave this as is. If you are using this as a > shorthand for writing 'state state', that's typically > written 'state state'. Same for delimType. > Actually for the latter I would write delim delimType. Done for both and I renamed delimType to delim. The first template language that I implemented this scheme for, my reviewer liked longIdentifierNames, but I will not bring that terminological baggage into future CLs. http://codereview.appspot.com/4906043/diff/2003/src/pkg/exp/template/html/con... src/pkg/exp/template/html/context.go:22: // GoString obeys the contract of fmt.Stringer. On 2011/08/16 22:13:33, rsc wrote: > can delete this. String is so common that we don't > bother commenting it. Done. http://codereview.appspot.com/4906043/diff/2003/src/pkg/exp/template/html/con... src/pkg/exp/template/html/context.go:53: switch s { On 2011/08/16 22:13:33, rsc wrote: > This is about at the limit where you'd want to use a switch. > If you think the state set will grow, better to use a table. > For example > http://golang.org/src/pkg/debug/dwarf/const.go#L164 This will grow over time. Replaced both switches with tables. http://codereview.appspot.com/4906043/diff/2003/src/pkg/exp/template/html/con... src/pkg/exp/template/html/context.go:63: return fmt.Sprintf("Illegal state %d", uint(s)) On 2011/08/16 22:13:33, rsc wrote: > usually lower case, because these get stuck into > the middle of other sentences. s/I/i/ Done in 2 places.
Sign in to reply to this message.
http://codereview.appspot.com/4906043/diff/11003/src/pkg/exp/template/html/co... File src/pkg/exp/template/html/context.go (right): http://codereview.appspot.com/4906043/diff/11003/src/pkg/exp/template/html/co... src/pkg/exp/template/html/context.go:23: return fmt.Sprintf("context{state: %s, delim: %s", I wouldn't break the statement over two lines. Go doesn't have a 80 char limit in general, only for doc comments (since people often run godoc in a terminal). http://codereview.appspot.com/4906043/diff/11003/src/pkg/exp/template/html/co... src/pkg/exp/template/html/context.go:38: // stateHtmlPcdata is parsed character data. An HTML parser is in IIUC, Go capitalization style would be "stateHTMLPCDATA" or "stateHTMLPCData". For example, xml defines MarshalXML, not MarshalXml. But rsc or r might override me in this case, since "stateHTMLPCDATA" looks awkward. http://codereview.appspot.com/4906043/diff/11003/src/pkg/exp/template/html/co... src/pkg/exp/template/html/context.go:42: // stateHtmlTag occurs before an HTML attribute or the end of a tag Missing full stop. Similarly below.
Sign in to reply to this message.
Incremental diffs at http://codereview.appspot.com/4906043/diff2/11003:10002/src/pkg/exp/template/... http://codereview.appspot.com/4906043/diff/11003/src/pkg/exp/template/html/co... File src/pkg/exp/template/html/context.go (right): http://codereview.appspot.com/4906043/diff/11003/src/pkg/exp/template/html/co... src/pkg/exp/template/html/context.go:23: return fmt.Sprintf("context{state: %s, delim: %s", On 2011/08/17 00:40:12, nigeltao wrote: > I wouldn't break the statement over two lines. Go doesn't have a 80 char limit > in general, only for doc comments (since people often run godoc in a terminal). Done. With delim instead of delimType it fits within 80 columns. http://codereview.appspot.com/4906043/diff/11003/src/pkg/exp/template/html/co... src/pkg/exp/template/html/context.go:38: // stateHtmlPcdata is parsed character data. An HTML parser is in On 2011/08/17 00:40:12, nigeltao wrote: > IIUC, Go capitalization style would be "stateHTMLPCDATA" or "stateHTMLPCData". > For example, xml defines MarshalXML, not MarshalXml. But rsc or r might override > me in this case, since "stateHTMLPCDATA" looks awkward. I'll wait for them to comment then? http://codereview.appspot.com/4906043/diff/11003/src/pkg/exp/template/html/co... src/pkg/exp/template/html/context.go:42: // stateHtmlTag occurs before an HTML attribute or the end of a tag On 2011/08/17 00:40:12, nigeltao wrote: > Missing full stop. Similarly below. Fixed in 7 places.
Sign in to reply to this message.
>> IIUC, Go capitalization style would be "stateHTMLPCDATA" or > > "stateHTMLPCData". >> >> For example, xml defines MarshalXML, not MarshalXml. But rsc or r > > might override >> >> me in this case, since "stateHTMLPCDATA" looks awkward. > > I'll wait for them to comment then? usually these run-ons are a sign of long-winded names anyway. you're in a package named html. you can drop it from most identifiers. i think statePCDATA or statePCData would be fine. russ
Sign in to reply to this message.
On Aug 17, 2011, at 12:06 PM, Russ Cox wrote: >>> IIUC, Go capitalization style would be "stateHTMLPCDATA" or >> >> "stateHTMLPCData". >>> >>> For example, xml defines MarshalXML, not MarshalXml. But rsc or r >> >> might override >>> >>> me in this case, since "stateHTMLPCDATA" looks awkward. >> >> I'll wait for them to comment then? > > usually these run-ons are a sign of > long-winded names anyway. > > you're in a package named html. > you can drop it from most identifiers. > > i think statePCDATA or statePCData > would be fine. agreed. i was about to make the same comment if i could get a network connection -rob
Sign in to reply to this message.
2011/8/16 Rob 'Commander' Pike <r@google.com>: > > On Aug 17, 2011, at 12:06 PM, Russ Cox wrote: > >>>> IIUC, Go capitalization style would be "stateHTMLPCDATA" or >>> >>> "stateHTMLPCData". >>>> >>>> For example, xml defines MarshalXML, not MarshalXml. But rsc or r >>> >>> might override >>>> >>>> me in this case, since "stateHTMLPCDATA" looks awkward. >>> >>> I'll wait for them to comment then? >> >> usually these run-ons are a sign of >> long-winded names anyway. >> >> you're in a package named html. >> you can drop it from most identifiers. >> >> i think statePCDATA or statePCData >> would be fine. > > agreed. i was about to make the same comment if i could get a network connection Change stateHtmlPcdata -> statePCDATA and stateHtmlTag -> stateTag. Incremental diffs at http://codereview.appspot.com/4906043/diff2/10002:16001/src/pkg/exp/template/...
Sign in to reply to this message.
a couple of tiny tweaks and it's ready http://codereview.appspot.com/4906043/diff/16001/src/pkg/exp/template/html/co... File src/pkg/exp/template/html/context.go (right): http://codereview.appspot.com/4906043/diff/16001/src/pkg/exp/template/html/co... src/pkg/exp/template/html/context.go:14: // context{} is the start context for a template that produces an HTML fragment this is correct but it's slightly clearer and more idiomatic to say this as The zero value of type context is the ... http://codereview.appspot.com/4906043/diff/16001/src/pkg/exp/template/html/co... src/pkg/exp/template/html/context.go:33: // action in a template. not sure how to parse that last sentence. is there a comma missing?
Sign in to reply to this message.
Incremental diffs at http://codereview.appspot.com/4906043/diff2/16001:19001/src/pkg/exp/template/... http://codereview.appspot.com/4906043/diff/16001/src/pkg/exp/template/html/co... File src/pkg/exp/template/html/context.go (right): http://codereview.appspot.com/4906043/diff/16001/src/pkg/exp/template/html/co... src/pkg/exp/template/html/context.go:14: // context{} is the start context for a template that produces an HTML fragment On 2011/08/17 03:02:24, r wrote: > this is correct but it's slightly clearer and more idiomatic to say this as > The zero value of type context is the ... Done. http://codereview.appspot.com/4906043/diff/16001/src/pkg/exp/template/html/co... src/pkg/exp/template/html/context.go:33: // action in a template. On 2011/08/17 03:02:24, r wrote: > not sure how to parse that last sentence. is there a comma missing? I reworded it. It's still long but I think it's clearer. // It bounds the top of the element stack, and by extension the HTML // insertion mode, but also contains state that does not correspond to // anything in the HTML5 parsing algorithm because a single token // production in the HTML grammar may contain embedded actions in a template, // e.g. the quoted HTML attribute produced <div title="Hello {{.World}}"> is a // single token in HTML's grammar but in a template spans several nodes.
Sign in to reply to this message.
apologies for the nitpicking. http://codereview.appspot.com/4906043/diff/19001/src/pkg/exp/template/html/co... File src/pkg/exp/template/html/context.go (right): http://codereview.appspot.com/4906043/diff/19001/src/pkg/exp/template/html/co... src/pkg/exp/template/html/context.go:33: // e.g. the quoted HTML attribute produced <div title="Hello {{.World}}"> is a i have a peeve about latinates, plus you can clean this up by breaking the sentence in two. after template on line 32, put a full stop. then, For instance, the quoted...
Sign in to reply to this message.
http://codereview.appspot.com/4906043/diff/19001/src/pkg/exp/template/html/co... File src/pkg/exp/template/html/context.go (right): http://codereview.appspot.com/4906043/diff/19001/src/pkg/exp/template/html/co... src/pkg/exp/template/html/context.go:33: // e.g. the quoted HTML attribute produced <div title="Hello {{.World}}"> is a On 2011/08/18 00:17:35, r wrote: > i have a peeve about latinates, plus you can clean this up by breaking the > sentence in two. after template on line 32, put a full stop. then, > > For instance, the quoted... By "latinates", do you mean "e.g.", "i.e.", "etc.", etc.? If so, done.
Sign in to reply to this message.
LGTM yes. it's not really the right term, but i use it for a shorthand describing abbreviations of latin phrases. why write latin? in any case, the comment is much clearer now.
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=9e0ce8b022f6 *** exp/template/html: defines a parse context for a subset of HTML. This defines just enough context to distinguish HTML URI attributes from parsed character data. It does not affect any public module API as I thought I would get early comment on style for defining enumerations and tables. R=rsc, r, nigeltao, r CC=golang-dev http://codereview.appspot.com/4906043 Committer: Rob Pike <r@golang.org>
Sign in to reply to this message.
|
