code review 27110043: go.net/html/charset: new package

go.net/html/charset: new package

Implement retrieving encodings by name, according to the names listed
at http://encoding.spec.whatwg.org/#encodings

This is the first step toward implementing the encoding detection
algorithm.

[andybalholm, 2013-11-16 00:38:57]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com,
nigeltao@golang.org),

I'd like you to review this change to
https://code.google.com/p/go.net

[nigeltao, 2013-11-16 02:31:46]

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding.go
File html/charset/encoding.go (right):

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding.go#n...
html/charset/encoding.go:1: // The charset package helps consumers of HTML to
implement the spec at
// Package charset helps etc.

but I find this whole sentence weak and of low information. Instead:

// Package charset provides common text encodings for HTML documents.
//
// The mapping from encoding names (and their aliases) to encodings is defined
// at http://encoding.spec.whatwg.org/

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding.go#n...
html/charset/encoding.go:14: "errors"
I like to separate standard library imports from other imports.

import (
  "errors"
  "strings"

  "code.google.com/etc0"
  "code.google.com/etc1"
)

Ditto with encoding_test.go.

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding.go#n...
html/charset/encoding.go:17: 
This exported function needs a doc comment.

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding.go#n...
html/charset/encoding.go:18: func GetEncoding(label string) (e
encoding.Encoding, ok bool) {
Is the ok necessary if it's always going to be (e != nil)?

I'd also drop the "Get".

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding.go#n...
html/charset/encoding.go:19: label = strings.ToLower(strings.Trim(label,
"\t\n\r\f "))
Are labels expected to have tabs or newlines??

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding.go#n...
html/charset/encoding.go:26: func (_ utf8Encoding) NewDecoder()
transform.Transformer {
You can drop the _.

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding.go#n...
html/charset/encoding.go:55: "unicode-1-1-utf-8":   utf8Encoding{},
This list comes from http://encoding.spec.whatwg.org/ yes? You should at least
add a comment to say that, but it might be nice if you could automate building
the table from that web page, in case that web page gets updated in the future.

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding_test.go
File html/charset/encoding_test.go (right):

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding_test...
html/charset/encoding_test.go:8: func transformString(t transform.Transformer, s
string) (string, error) {
Would this be easier using io.Copy into a bytes.Buffer?

I'd also drop the "String".

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding_test...
html/charset/encoding_test.go:79: for _, data := range testData {
Personally, I'd rename data and testData to tc and testCases.

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding_test...
html/charset/encoding_test.go:94: t.Errorf("Unexpected value: %#v (expected
%#v), decoding %s", str, data.utf8, data.otherEncoding)
I'd say

t.Errorf("%s: got %q, want %q", tc.otherEncoding, s, tc.utf8)

[nigeltao, 2013-11-16 02:35:32]

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding.go
File html/charset/encoding.go (right):

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding.go#n...
html/charset/encoding.go:55: "unicode-1-1-utf-8":   utf8Encoding{},
Specifically, http://encoding.spec.whatwg.org/encodings.json

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding_test.go
File html/charset/encoding_test.go (right):

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding_test...
html/charset/encoding_test.go:8: func transformString(t transform.Transformer, s
string) (string, error) {
On 2013/11/16 02:31:46, nigeltao wrote:
> Would this be easier using io.Copy into a bytes.Buffer?

Or even better, ioutil.ReadAll?

[andybalholm, 2013-11-16 18:33:10]

PTAL.

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding.go
File html/charset/encoding.go (right):

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding.go#n...
html/charset/encoding.go:1: // The charset package helps consumers of HTML to
implement the spec at
On 2013/11/16 02:31:46, nigeltao wrote:
> // Package charset helps etc.
> 
> but I find this whole sentence weak and of low information. Instead:
> 
> // Package charset provides common text encodings for HTML documents.
> //
> // The mapping from encoding names (and their aliases) to encodings is defined
> // at http://encoding.spec.whatwg.org/

Done.

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding.go#n...
html/charset/encoding.go:14: "errors"
On 2013/11/16 02:31:46, nigeltao wrote:
> I like to separate standard library imports from other imports.
> 
> import (
>   "errors"
>   "strings"
> 
>   "code.google.com/etc0"
>   "code.google.com/etc1"
> )
> 
> Ditto with encoding_test.go.

Done.

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding.go#n...
html/charset/encoding.go:17: 
On 2013/11/16 02:31:46, nigeltao wrote:
> This exported function needs a doc comment.

Done.

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding.go#n...
html/charset/encoding.go:18: func GetEncoding(label string) (e
encoding.Encoding, ok bool) {
On 2013/11/16 02:31:46, nigeltao wrote:
> Is the ok necessary if it's always going to be (e != nil)?
> 
> I'd also drop the "Get".

Done.

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding.go#n...
html/charset/encoding.go:19: label = strings.ToLower(strings.Trim(label,
"\t\n\r\f "))
On 2013/11/16 02:31:46, nigeltao wrote:
> Are labels expected to have tabs or newlines??

or form-feeds? :-)

But the spec says: 
    Remove any leading and trailing ASCII whitespace from label.

And it defines "ASCII whitespace" as:
    code points U+0009, U+000A, U+000C, U+000D, and U+0020

So that's what I did. Making sense and complying with HTML specs are sometimes
mutually exclusive.

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding.go#n...
html/charset/encoding.go:26: func (_ utf8Encoding) NewDecoder()
transform.Transformer {
On 2013/11/16 02:31:46, nigeltao wrote:
> You can drop the _.

Done.

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding.go#n...
html/charset/encoding.go:55: "unicode-1-1-utf-8":   utf8Encoding{},
On 2013/11/16 02:31:46, nigeltao wrote:
> This list comes from http://encoding.spec.whatwg.org/ yes? You should at least
> add a comment to say that, but it might be nice if you could automate building
> the table from that web page, in case that web page gets updated in the
future.

Done.

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding_test.go
File html/charset/encoding_test.go (right):

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding_test...
html/charset/encoding_test.go:8: func transformString(t transform.Transformer, s
string) (string, error) {
On 2013/11/16 02:31:46, nigeltao wrote:
> Would this be easier using io.Copy into a bytes.Buffer?
> 
> I'd also drop the "String".

But we're already importing a package named "transform".

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding_test...
html/charset/encoding_test.go:8: func transformString(t transform.Transformer, s
string) (string, error) {
On 2013/11/16 02:35:32, nigeltao wrote:
> On 2013/11/16 02:31:46, nigeltao wrote:
> > Would this be easier using io.Copy into a bytes.Buffer?
> 
> Or even better, ioutil.ReadAll?

Done.

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding_test...
html/charset/encoding_test.go:79: for _, data := range testData {
On 2013/11/16 02:31:46, nigeltao wrote:
> Personally, I'd rename data and testData to tc and testCases.

Done.

https://codereview.appspot.com/27110043/diff/40001/html/charset/encoding_test...
html/charset/encoding_test.go:94: t.Errorf("Unexpected value: %#v (expected
%#v), decoding %s", str, data.utf8, data.otherEncoding)
On 2013/11/16 02:31:46, nigeltao wrote:
> I'd say
> 
> t.Errorf("%s: got %q, want %q", tc.otherEncoding, s, tc.utf8)

Done.

[nigeltao, 2013-11-18 04:16:18]

https://codereview.appspot.com/27110043/diff/50001/html/charset/encoding.go
File html/charset/encoding.go (right):

https://codereview.appspot.com/27110043/diff/50001/html/charset/encoding.go#n...
html/charset/encoding.go:3: // The mapping from encoding names (and their
aliases) to encodings is defined
Sorry for contradicting my previous self, but changing "names (and their
aliases)" to "labels" would probably be more consistent with the spec and the
rest of the exported API.

https://codereview.appspot.com/27110043/diff/50001/html/charset/encoding.go#n...
html/charset/encoding.go:16: // not one of the standard encodings for HTML.
The doc comment should at least say whether the label is case-sensitive, and
then you might as well say it trims ASCII whitespace.

https://codereview.appspot.com/27110043/diff/50001/html/charset/encoding.go#n...
html/charset/encoding.go:21: func normalizeLabel(label string) string {
I don't see why this has to be its own function.

https://codereview.appspot.com/27110043/diff/50001/html/charset/encoding.go#n...
html/charset/encoding.go:35: var ErrReplacementEncoding =
errors.New("ISO-2022-CN and ISO-2022-KR are forbidden in HTML")
I'd prefix the error message with the package name, so that people who encounter
this error have a bit more context to grep for:

var ErrReplacementEncoding = errors.New("charset: ISO-2022-CN and ISO-2022-KR
are obsolete HTML encodings")

https://codereview.appspot.com/27110043/diff/50001/html/charset/encoding_test.go
File html/charset/encoding_test.go (right):

https://codereview.appspot.com/27110043/diff/50001/html/charset/encoding_test...
html/charset/encoding_test.go:67: t.Errorf("Could not create decoder for %s",
tc.otherEncoding)
I would make these error messages consistently be prefixed by tc.otherEncoding:

t.Errorf("%s: not found", tc.otherEncoding)

and below:

t.Errorf("%s: decode %q: %v", tc.otherEncoding, tc.other, err)

https://codereview.appspot.com/27110043/diff/50001/html/charset/gen.go
File html/charset/gen.go (right):

https://codereview.appspot.com/27110043/diff/50001/html/charset/gen.go#newcode3
html/charset/gen.go:3: // Download
http://encoding.spec.whatwg.org/encodings.json and use it to
I'd put this comment below the package clause, instead of being a doc comment,
similarly to go.net/publicsuffix's gen.go.

https://codereview.appspot.com/27110043/diff/50001/html/charset/gen.go#newcode4
html/charset/gen.go:4: // generate encodinglist.go.
I'd shorten encodinglist.go to table.go, similarly to go.net/publicsuffix. It's
not like there's going to be any other sorts of lists in this package.

https://codereview.appspot.com/27110043/diff/50001/html/charset/gen.go#newcode37
html/charset/gen.go:37: var encodings []encGroup
It might read better if this was
var groups []group

https://codereview.appspot.com/27110043/diff/50001/html/charset/gen.go#newcode44
html/charset/gen.go:44: fmt.Println("// This file is automatically generated
from", specURL)
I'd just say
"// generated by go run gen.go; DO NOT EDIT"
a la go.net/publicsuffix.

https://codereview.appspot.com/27110043/diff/50001/html/charset/gen.go#newcode59
html/charset/gen.go:59: for _, grp := range encodings {
I'd write
for _, g := range groups {

[andybalholm, 2013-11-18 17:01:27]

PTAL.

https://codereview.appspot.com/27110043/diff/50001/html/charset/encoding.go
File html/charset/encoding.go (right):

https://codereview.appspot.com/27110043/diff/50001/html/charset/encoding.go#n...
html/charset/encoding.go:3: // The mapping from encoding names (and their
aliases) to encodings is defined
On 2013/11/18 04:16:18, nigeltao wrote:
> Sorry for contradicting my previous self, but changing "names (and their
> aliases)" to "labels" would probably be more consistent with the spec and the
> rest of the exported API.

Done.

https://codereview.appspot.com/27110043/diff/50001/html/charset/encoding.go#n...
html/charset/encoding.go:16: // not one of the standard encodings for HTML.
On 2013/11/18 04:16:18, nigeltao wrote:
> The doc comment should at least say whether the label is case-sensitive, and
> then you might as well say it trims ASCII whitespace.

Done.

https://codereview.appspot.com/27110043/diff/50001/html/charset/encoding.go#n...
html/charset/encoding.go:21: func normalizeLabel(label string) string {
On 2013/11/18 04:16:18, nigeltao wrote:
> I don't see why this has to be its own function.

At one point it looked like I would be doing this in two places. But now it's
only one. So I'll inline it.

https://codereview.appspot.com/27110043/diff/50001/html/charset/encoding.go#n...
html/charset/encoding.go:35: var ErrReplacementEncoding =
errors.New("ISO-2022-CN and ISO-2022-KR are forbidden in HTML")
On 2013/11/18 04:16:18, nigeltao wrote:
> I'd prefix the error message with the package name, so that people who
encounter
> this error have a bit more context to grep for:
> 
> var ErrReplacementEncoding = errors.New("charset: ISO-2022-CN and ISO-2022-KR
> are obsolete HTML encodings")

Done.

https://codereview.appspot.com/27110043/diff/50001/html/charset/encoding_test.go
File html/charset/encoding_test.go (right):

https://codereview.appspot.com/27110043/diff/50001/html/charset/encoding_test...
html/charset/encoding_test.go:67: t.Errorf("Could not create decoder for %s",
tc.otherEncoding)
On 2013/11/18 04:16:18, nigeltao wrote:
> I would make these error messages consistently be prefixed by
tc.otherEncoding:
> 
> t.Errorf("%s: not found", tc.otherEncoding)
> 
> and below:
> 
> t.Errorf("%s: decode %q: %v", tc.otherEncoding, tc.other, err)

Done.

https://codereview.appspot.com/27110043/diff/50001/html/charset/gen.go
File html/charset/gen.go (right):

https://codereview.appspot.com/27110043/diff/50001/html/charset/gen.go#newcode37
html/charset/gen.go:37: var encodings []encGroup
On 2013/11/18 04:16:18, nigeltao wrote:
> It might read better if this was
> var groups []group

Done.

https://codereview.appspot.com/27110043/diff/50001/html/charset/gen.go#newcode44
html/charset/gen.go:44: fmt.Println("// This file is automatically generated
from", specURL)
On 2013/11/18 04:16:18, nigeltao wrote:
> I'd just say
> "// generated by go run gen.go; DO NOT EDIT"
> a la go.net/publicsuffix.

Done.

https://codereview.appspot.com/27110043/diff/50001/html/charset/gen.go#newcode59
html/charset/gen.go:59: for _, grp := range encodings {
On 2013/11/18 04:16:18, nigeltao wrote:
> I'd write
> for _, g := range groups {

Done.

[nigeltao, 2013-11-19 10:49:31]

LGTM.

Thanks for your patience with my nit-picking.

[nigeltao, 2013-11-19 10:53:15]

*** Submitted as
https://code.google.com/p/go/source/detail?r=c31ae4e5b710&repo=net ***

go.net/html/charset: new package

Implement retrieving encodings by name, according to the names listed
at http://encoding.spec.whatwg.org/#encodings

This is the first step toward implementing the encoding detection
algorithm.

R=nigeltao
CC=golang-dev
https://codereview.appspot.com/27110043

Committer: Nigel Tao <nigeltao@golang.org>