Hello rsc@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg
https://codereview.appspot.com/7066049/diff/2001/src/pkg/exp/locale/locale.go File src/pkg/exp/locale/locale.go (right): https://codereview.appspot.com/7066049/diff/2001/src/pkg/exp/locale/locale.go... src/pkg/exp/locale/locale.go:26: // Locale represents a Unicode Locale Identifier. It can by various packages to s/by various packages/be used/ https://codereview.appspot.com/7066049/diff/2001/src/pkg/exp/locale/locale.go... src/pkg/exp/locale/locale.go:38: // http://www.unicode.org/reports/tr35/#Unicode_Language_and_Locale_Identifiers. remove full stop at the end? https://codereview.appspot.com/7066049/diff/2001/src/pkg/exp/locale/locale.go... src/pkg/exp/locale/locale.go:76: type PartType int type Part int maybe? PartType sounds strange. On second thought, why not make something like type Descriptor struct { Identifier, Language, Script, Region, Variant, Transform string KeyValue map[string]string // ? Attributes []string PrivateUse string } https://codereview.appspot.com/7066049/diff/2001/src/pkg/exp/locale/locale.go... src/pkg/exp/locale/locale.go:124: // by default from it's parent. its https://codereview.appspot.com/7066049/diff/2001/src/pkg/exp/locale/locale.go... src/pkg/exp/locale/locale.go:146: type Script struct { type Script string? same below
https://codereview.appspot.com/7066049/diff/2001/src/pkg/exp/locale/locale.go File src/pkg/exp/locale/locale.go (right): https://codereview.appspot.com/7066049/diff/2001/src/pkg/exp/locale/locale.go... src/pkg/exp/locale/locale.go:26: // Locale represents a Unicode Locale Identifier. It can by various packages to On 2013/01/07 20:35:06, bsiegert wrote: > s/by various packages/be used/ Done. https://codereview.appspot.com/7066049/diff/2001/src/pkg/exp/locale/locale.go... src/pkg/exp/locale/locale.go:38: // http://www.unicode.org/reports/tr35/#Unicode_Language_and_Locale_Identifiers. Reworked. On 2013/01/07 20:35:06, bsiegert wrote: > remove full stop at the end? https://codereview.appspot.com/7066049/diff/2001/src/pkg/exp/locale/locale.go... src/pkg/exp/locale/locale.go:76: type PartType int Part is indeed better. The Descriptor makes more sense if Language etc. are string instead of struct, but I'll consider it. On 2013/01/07 20:35:06, bsiegert wrote: > type Part int maybe? PartType sounds strange. > > On second thought, why not make something like > > type Descriptor struct { > Identifier, Language, Script, Region, Variant, Transform string > KeyValue map[string]string // ? > Attributes []string > PrivateUse string > } https://codereview.appspot.com/7066049/diff/2001/src/pkg/exp/locale/locale.go... src/pkg/exp/locale/locale.go:76: type PartType int Changed to Part. The Descriptor make more sense if the various types are string instead of struct. It only makes sense for setting. Also, identifier overlaps with language, script, region and variant. But I'll consider it. On 2013/01/07 20:35:06, bsiegert wrote: > type Part int maybe? PartType sounds strange. > > On second thought, why not make something like > > type Descriptor struct { > Identifier, Language, Script, Region, Variant, Transform string > KeyValue map[string]string // ? > Attributes []string > PrivateUse string > } https://codereview.appspot.com/7066049/diff/2001/src/pkg/exp/locale/locale.go... src/pkg/exp/locale/locale.go:124: // by default from it's parent. On 2013/01/07 20:35:06, bsiegert wrote: > its Done. https://codereview.appspot.com/7066049/diff/2001/src/pkg/exp/locale/locale.go... src/pkg/exp/locale/locale.go:146: type Script struct { This is to be able to get some guarantee the string is well-formed. I'm indeed considering changing this, though, but will leave it like this for now as changing it to string is irreversible. On 2013/01/07 20:35:06, bsiegert wrote: > type Script string? same below
R=close