code review 157104: Fixed a problem of xml module

As your adrress, I dropped invalid letters to make a variable name.
But I made new function named 'Filter' instead of using 'Map'
with multipurpose like returning '-1' to drop letter or byte.
Cause map is map. As far as I know that commonly map returns
revised list with same size.

But another things arre guided your last advise. thank you.

[hey, 2009-11-20 08:57:26]

Hello r,

I'd like you to review the following change.

[r, 2009-11-20 18:44:53]

changing reviewer to rsc

[rsc, 2009-11-20 21:00:10]

Thanks for suggesting this.  A few comments below.
Please also update xml_test.go to exercise this code.

http://codereview.appspot.com/157104/diff/1002/3
File src/pkg/xml/read.go (right):

http://codereview.appspot.com/157104/diff/1002/3#newcode150
src/pkg/xml/read.go:150: return '_'
I'm not wild about putting _ in, and presumably the
test should be for more than just '-', even though - 
is the common problem.

I suggest changing strings.Map and bytes.Map to treat
the mapping function returning < 0 as meaning "drop the letter".
Then this test can be:

if rune != '_' && !unicode.IsDigit(rune) && !unicode.IsAlpha(rune) {
    return -1;
}

[hey, 2009-11-21 03:19:01]

ok. thanks for good suggestion. 

and also I probably should do handle first letter of attribute especially when
it is a  number or an underscore.  Isn't it?
 
On 2009/11/20 21:00:10, rsc wrote:
> Thanks for suggesting this.  A few comments below.
> Please also update xml_test.go to exercise this code.
> 
> http://codereview.appspot.com/157104/diff/1002/3
> File src/pkg/xml/read.go (right):
> 
> http://codereview.appspot.com/157104/diff/1002/3#newcode150
> src/pkg/xml/read.go:150: return '_'
> I'm not wild about putting _ in, and presumably the
> test should be for more than just '-', even though - 
> is the common problem.
> 
> I suggest changing strings.Map and bytes.Map to treat
> the mapping function returning < 0 as meaning "drop the letter".
> Then this test can be:
> 
> if rune != '_' && !unicode.IsDigit(rune) && !unicode.IsAlpha(rune) {
>     return -1;
> }

[hey, 2009-11-25 04:40:28]

As your adrress, I dropped invalid letters to make a variable name.
But I made new function named 'Filter' instead of using 'Map'
with multipurpose like returning '-1' to drop letter or byte.
Cause map is map. As far as I know that commonly map returns
revised list with same size.

But another things arre guided your last advise. thank you.

On 2009/11/21 03:19:01, hey wrote:
> ok. thanks for good suggestion. 
> 
> and also I probably should do handle first letter of attribute especially when
> it is a  number or an underscore.  Isn't it?
>  
> On 2009/11/20 21:00:10, rsc wrote:
> > Thanks for suggesting this.  A few comments below.
> > Please also update xml_test.go to exercise this code.
> > 
> > http://codereview.appspot.com/157104/diff/1002/3
> > File src/pkg/xml/read.go (right):
> > 
> > http://codereview.appspot.com/157104/diff/1002/3#newcode150
> > src/pkg/xml/read.go:150: return '_'
> > I'm not wild about putting _ in, and presumably the
> > test should be for more than just '-', even though - 
> > is the common problem.
> > 
> > I suggest changing strings.Map and bytes.Map to treat
> > the mapping function returning < 0 as meaning "drop the letter".
> > Then this test can be:
> > 
> > if rune != '_' && !unicode.IsDigit(rune) && !unicode.IsAlpha(rune) {
> >     return -1;
> > }

[hey, 2009-11-25 04:41:07]

PTAL

[hey, 2009-11-30 01:27:18]

PTAL

[rsc, 2009-11-30 02:11:24]

I still think it would be better to define Map's
behavior when the function returns < 0.
That's strictly more powerful than either the
Map or Filter we have right now and lets you
do FieldName (aka Validate) in one go.

http://codereview.appspot.com/157104/diff/3009/4005
File src/pkg/xml/read.go (right):

http://codereview.appspot.com/157104/diff/3009/4005#newcode150
src/pkg/xml/read.go:150: func Validated(original string) string {
Maybe s/Validated/FieldName/?

The comment should be a complete sentence (see
http://golang.org/doc/effective_go.html#commentary).

This actually isn't the field name (it is a lowercase
version of the field name), so maybe func fieldName,
i.e. not public.

http://codereview.appspot.com/157104/diff/3009/4005#newcode151
src/pkg/xml/read.go:151: escaped := strings.Filter(func(x int) bool { return (x
== '_') || unicode.IsDigit(x) || unicode.IsLetter(x) },
No need for parens around the x == '_'

http://codereview.appspot.com/157104/diff/3009/4006
File src/pkg/xml/read_test.go (right):

http://codereview.appspot.com/157104/diff/3009/4006#newcode212
src/pkg/xml/read_test.go:212: type ValidatedTest struct {
,s/Validated/FieldName/

[hey, 2009-11-30 02:47:33]

Yes, it seems more powerful. But, meaning of map is known different with that in
many other language specs. If you still want to treat it like that, I suggest
change the function's name to something else.

On 2009/11/30 02:11:24, rsc wrote:
> I still think it would be better to define Map's
> behavior when the function returns < 0.
> That's strictly more powerful than either the
> Map or Filter we have right now and lets you
> do FieldName (aka Validate) in one go.
> 
> http://codereview.appspot.com/157104/diff/3009/4005
> File src/pkg/xml/read.go (right):
> 
> http://codereview.appspot.com/157104/diff/3009/4005#newcode150
> src/pkg/xml/read.go:150: func Validated(original string) string {
> Maybe s/Validated/FieldName/?
> 
> The comment should be a complete sentence (see
> http://golang.org/doc/effective_go.html#commentary).
> 
> This actually isn't the field name (it is a lowercase
> version of the field name), so maybe func fieldName,
> i.e. not public.
> 
> http://codereview.appspot.com/157104/diff/3009/4005#newcode151
> src/pkg/xml/read.go:151: escaped := strings.Filter(func(x int) bool { return
(x
> == '_') || unicode.IsDigit(x) || unicode.IsLetter(x) },
> No need for parens around the x == '_'
> 
> http://codereview.appspot.com/157104/diff/3009/4006
> File src/pkg/xml/read_test.go (right):
> 
> http://codereview.appspot.com/157104/diff/3009/4006#newcode212
> src/pkg/xml/read_test.go:212: type ValidatedTest struct {
> ,s/Validated/FieldName/

[rsc, 2009-11-30 02:56:49]

On Sun, Nov 29, 2009 at 18:47,  <hey.calmdown@gmail.com> wrote:
> Yes, it seems more powerful. But, meaning of map is known different with
> that in many other language specs. If you still want to treat it like
> that, I suggest change the function's name to something else.

I think this fits just fine.
It's already doing Map + concatenate.
It's just that the string being concatenated
for f(c) < 0 is "" instead of string(f(c)).

Russ

[hey, 2009-11-30 05:17:05]

ok.

I fixed it.
Thanks.

http://codereview.appspot.com/157104/diff/3009/4005
File src/pkg/xml/read.go (right):

http://codereview.appspot.com/157104/diff/3009/4005#newcode150
src/pkg/xml/read.go:150: func Validated(original string) string {
On 2009/11/30 02:11:24, rsc wrote:
> Maybe s/Validated/FieldName/?
> 
> The comment should be a complete sentence (see
> http://golang.org/doc/effective_go.html#commentary).
> 
> This actually isn't the field name (it is a lowercase
> version of the field name), so maybe func fieldName,
> i.e. not public.
> 

Done.

http://codereview.appspot.com/157104/diff/3009/4005#newcode151
src/pkg/xml/read.go:151: escaped := strings.Filter(func(x int) bool { return (x
== '_') || unicode.IsDigit(x) || unicode.IsLetter(x) },
On 2009/11/30 02:11:24, rsc wrote:
> No need for parens around the x == '_'
> 

Done.

http://codereview.appspot.com/157104/diff/3009/4006
File src/pkg/xml/read_test.go (right):

http://codereview.appspot.com/157104/diff/3009/4006#newcode212
src/pkg/xml/read_test.go:212: type ValidatedTest struct {
On 2009/11/30 02:11:24, rsc wrote:
> ,s/Validated/FieldName/
> 

Done.

http://codereview.appspot.com/157104/diff/3009/4006#newcode212
src/pkg/xml/read_test.go:212: type ValidatedTest struct {
On 2009/11/30 02:11:24, rsc wrote:
> ,s/Validated/FieldName/
> 

Done.

[rsc, 2009-11-30 05:44:47]

looking pretty good

http://codereview.appspot.com/157104/diff/5005/5006
File src/pkg/bytes/bytes.go (right):

http://codereview.appspot.com/157104/diff/5005/5006#newcode209
src/pkg/bytes/bytes.go:209: func Filter(filter func(int) bool, s []byte) []byte
{
Now filter can go away.

http://codereview.appspot.com/157104/diff/5005/5006#newcode228
src/pkg/bytes/bytes.go:228: // according to the mapping function. If mapping
function returns -1 against a byte
If mapping returns a negative value, the character is
dropped from the string with no replacement.

http://codereview.appspot.com/157104/diff/5005/5006#newcode244
src/pkg/bytes/bytes.go:244: if -1 < rune {
The Go idiom is to write comparisons the normal way.
Please change to if rune >= 0.

I know there are some people who advocate this weird
reversal in C to catch an error if you type if -1 = rune
instead of if -1 == rune, but the former is a syntax
error in Go, so it's not necessary.  In Go it's just weird.

http://codereview.appspot.com/157104/diff/5005/5007
File src/pkg/bytes/bytes_test.go (right):

http://codereview.appspot.com/157104/diff/5005/5007#newcode417
src/pkg/bytes/bytes_test.go:417: type FilterTest struct {
Can drop this now - instead of Filter people
can use Map.

http://codereview.appspot.com/157104/diff/5005/5008
File src/pkg/strings/strings.go (right):

http://codereview.appspot.com/157104/diff/5005/5008#newcode180
src/pkg/strings/strings.go:180: // Filter returns a copy of the string s with
its filtered characters which fits
drop

http://codereview.appspot.com/157104/diff/5005/5008#newcode195
src/pkg/strings/strings.go:195: // according to the mapping function. If mapping
function returns -1 against a letter
If mapping returns a negative value, the character is
dropped from the string with no replacement.

http://codereview.appspot.com/157104/diff/5005/5008#newcode206
src/pkg/strings/strings.go:206: if -1 < rune {
rune >= 0

http://codereview.appspot.com/157104/diff/5005/5009
File src/pkg/strings/strings_test.go (right):

http://codereview.appspot.com/157104/diff/5005/5009#newcode387
src/pkg/strings/strings_test.go:387: type FilterTest struct {
drop

http://codereview.appspot.com/157104/diff/5005/5010
File src/pkg/xml/read.go (right):

http://codereview.appspot.com/157104/diff/5005/5010#newcode148
src/pkg/xml/read.go:148: // Escapes an original string to make it without
invalid characters which
// fieldName strips invalid characters from an XML name
// to create a valid Go struct name.  It also converts the
// name to lower case letters.

http://codereview.appspot.com/157104/diff/5005/5010#newcode151
src/pkg/xml/read.go:151: return strings.Filter(func(x int) bool { return
unicode.IsDigit(x) || unicode.IsLetter(x) },
change to use Map

http://codereview.appspot.com/157104/diff/5005/5011
File src/pkg/xml/read_test.go (right):

http://codereview.appspot.com/157104/diff/5005/5011#newcode28
src/pkg/xml/read_test.go:28: </title><link
href="http://codereview.appspot.com/126085"
rel="alternate"></link><updated>2009-10-04T01:35:58+00:00</updated><author><name>email-address-removed</name></author><id>urn:md5:134d9179c41f806be79b3a5f7877d19a</id><summary
type="html">
Change the <link and </link> on this line
to be <li-nk and </li-nk> in order to test
that fieldName is being used correctly by the
XML parser.

(The new test at the bottom only tests that
fieldName itself is correct, not that it gets used.)

[hey, 2009-11-30 06:12:26]

Thanks, I've done except one. :)
Please check my final comment.

http://codereview.appspot.com/157104/diff/5005/5006
File src/pkg/bytes/bytes.go (right):

http://codereview.appspot.com/157104/diff/5005/5006#newcode209
src/pkg/bytes/bytes.go:209: func Filter(filter func(int) bool, s []byte) []byte
{
On 2009/11/30 05:44:47, rsc wrote:
> Now filter can go away.
> 

Done.

http://codereview.appspot.com/157104/diff/5005/5006#newcode228
src/pkg/bytes/bytes.go:228: // according to the mapping function. If mapping
function returns -1 against a byte
On 2009/11/30 05:44:47, rsc wrote:
> If mapping returns a negative value, the character is
> dropped from the string with no replacement.
> 

Done.

http://codereview.appspot.com/157104/diff/5005/5006#newcode244
src/pkg/bytes/bytes.go:244: if -1 < rune {
On 2009/11/30 05:44:47, rsc wrote:
> The Go idiom is to write comparisons the normal way.
> Please change to if rune >= 0.
> 
> I know there are some people who advocate this weird
> reversal in C to catch an error if you type if -1 = rune
> instead of if -1 == rune, but the former is a syntax
> error in Go, so it's not necessary.  In Go it's just weird.

Done.

http://codereview.appspot.com/157104/diff/5005/5007
File src/pkg/bytes/bytes_test.go (right):

http://codereview.appspot.com/157104/diff/5005/5007#newcode417
src/pkg/bytes/bytes_test.go:417: type FilterTest struct {
On 2009/11/30 05:44:47, rsc wrote:
> Can drop this now - instead of Filter people
> can use Map.
> 

Done.

http://codereview.appspot.com/157104/diff/5005/5008
File src/pkg/strings/strings.go (right):

http://codereview.appspot.com/157104/diff/5005/5008#newcode180
src/pkg/strings/strings.go:180: // Filter returns a copy of the string s with
its filtered characters which fits
On 2009/11/30 05:44:47, rsc wrote:
> drop
> 

Done.

http://codereview.appspot.com/157104/diff/5005/5008#newcode195
src/pkg/strings/strings.go:195: // according to the mapping function. If mapping
function returns -1 against a letter
On 2009/11/30 05:44:47, rsc wrote:
> If mapping returns a negative value, the character is
> dropped from the string with no replacement.
> 

Done.

http://codereview.appspot.com/157104/diff/5005/5008#newcode206
src/pkg/strings/strings.go:206: if -1 < rune {
On 2009/11/30 05:44:47, rsc wrote:
> rune >= 0
> 

Done.

http://codereview.appspot.com/157104/diff/5005/5009
File src/pkg/strings/strings_test.go (right):

http://codereview.appspot.com/157104/diff/5005/5009#newcode387
src/pkg/strings/strings_test.go:387: type FilterTest struct {
On 2009/11/30 05:44:47, rsc wrote:
> drop

Done.

http://codereview.appspot.com/157104/diff/5005/5010
File src/pkg/xml/read.go (right):

http://codereview.appspot.com/157104/diff/5005/5010#newcode148
src/pkg/xml/read.go:148: // Escapes an original string to make it without
invalid characters which
On 2009/11/30 05:44:47, rsc wrote:
> // fieldName strips invalid characters from an XML name
> // to create a valid Go struct name.  It also converts the
> // name to lower case letters.
> 

Done.

http://codereview.appspot.com/157104/diff/5005/5010#newcode151
src/pkg/xml/read.go:151: return strings.Filter(func(x int) bool { return
unicode.IsDigit(x) || unicode.IsLetter(x) },
On 2009/11/30 05:44:47, rsc wrote:
> change to use Map
> 

Done.

http://codereview.appspot.com/157104/diff/5005/5011
File src/pkg/xml/read_test.go (right):

http://codereview.appspot.com/157104/diff/5005/5011#newcode28
src/pkg/xml/read_test.go:28: </title><link
href="http://codereview.appspot.com/126085"
rel="alternate"></link><updated>2009-10-04T01:35:58+00:00</updated><author><name>email-address-removed</name></author><id>urn:md5:134d9179c41f806be79b3a5f7877d19a</id><summary
type="html">
I just modified for attribute's name. Not tag's name. Where is the bunch of code
to create tag's name?

On 2009/11/30 05:44:47, rsc wrote:
> Change the <link and </link> on this line
> to be <li-nk and </li-nk> in order to test
> that fieldName is being used correctly by the
> XML parser.
> 
> (The new test at the bottom only tests that
> fieldName itself is correct, not that it gets used.)

[rsc, 2009-11-30 18:37:36]

> I just modified for attribute's name. Not tag's name. Where is the bunch
> of code to create tag's name?

it would be good to change an attribute name too, then.
like re-l="alternate".

the tag name code is a few lines past the attribute code

		case StartElement:
			// Sub-element.
			// Look up by tag name.
			// If that fails, fall back to mop-up field named "Any".
			if sv != nil {
				k := strings.ToLower(t.Name.Local);
				any := -1;

russ

[hey, 2009-12-01 01:33:45]

ok. I've done.

[hey, 2009-12-03 12:29:47]

PTAL

Is it ok?

[rsc, 2009-12-03 20:56:13]

looks good.  one final nit

http://codereview.appspot.com/157104/diff/7001/7006
File src/pkg/xml/read.go (right):

http://codereview.appspot.com/157104/diff/7001/7006#newcode155
src/pkg/xml/read.go:155: return x
return unicode.ToLower(x);

then you can pass original not strings.ToLower(original).
Might as well do one pass.

[hey, 2009-12-07 00:56:31]

Ok. good point. Thanks.

I've done.

http://codereview.appspot.com/157104/diff/7001/7006
File src/pkg/xml/read.go (right):

http://codereview.appspot.com/157104/diff/7001/7006#newcode155
src/pkg/xml/read.go:155: return x
On 2009/12/03 20:56:13, rsc wrote:
> return unicode.ToLower(x);
> 
> then you can pass original not strings.ToLower(original).
> Might as well do one pass.
> 

Done.

[rsc, 2009-12-07 18:30:59]

LGTM

thanks

[rsc, 2009-12-07 18:34:11]

please complete the appropriate CLA as described at
http://golang.org/doc/contribute.html#copyright.
don't bother changing AUTHORS or CONTRIBUTORS -
we'll take care of that - but do let me know once you've
filled out the form.

thanks.
russ

[hey, 2009-12-08 00:49:26]

I filled it. Thanks for addressed. :)

On 2009/12/07 18:34:11, rsc wrote:
> please complete the appropriate CLA as described at
> http://golang.org/doc/contribute.html#copyright.
> don't bother changing AUTHORS or CONTRIBUTORS -
> we'll take care of that - but do let me know once you've
> filled out the form.
> 
> thanks.
> russ

[rsc, 2009-12-09 01:25:21]

On 2009/12/08 00:49:26, hey wrote:
> I filled it. Thanks for addressed. :)

I can't find it.  Did you use your gmail address
hey.calmdown@gmail.com?

Russ

[hey, 2009-12-09 01:29:09]

Sorry, I filled again.
Maybe I wrote as wrong address.

Kei
On 2009/12/09 01:25:21, rsc wrote:
> On 2009/12/08 00:49:26, hey wrote:
> > I filled it. Thanks for addressed. :)
> 
> I can't find it.  Did you use your gmail address
> hey.calmdown@gmail.com?
> 
> Russ

[rsc, 2009-12-11 18:38:00]

*** Submitted as http://code.google.com/p/go/source/detail?r=ee32ba55347b ***

bytes, strings: allow -1 in Map to mean "drop this character".

xml: drop invalid characters in attribute names
    when constructing struct field names.

R=rsc
CC=r
http://codereview.appspot.com/157104

Committer: Russ Cox <rsc@golang.org>