Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: implement Hash.String #33430

Closed
Ma124 opened this issue Aug 2, 2019 · 13 comments
Closed

crypto: implement Hash.String #33430

Ma124 opened this issue Aug 2, 2019 · 13 comments

Comments

@Ma124
Copy link

@Ma124 Ma124 commented Aug 2, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12.1 linux/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ma_124/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ma_124/.go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build821474858=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I wanted to iterate all builtin hash functions by doing something like the following:

var algs map[string]crypto.Hash

var h crypto.Hash
for ; h < crypto.maxHash; h++ {
    if h.Available() {
        algs[h.String()] = h
    }
}

What did you expect to see?

An exported crypto.maxHash and crypto.Hash implementing the fmt.Stringer interface.

@odeke-em odeke-em changed the title crypto: crypto.Hash implement fmt.Stringer and export crypto.maxHash proposal: crypto: crypto.Hash implement fmt.Stringer and export crypto.maxHash Aug 3, 2019
@gopherbot gopherbot added this to the Proposal milestone Aug 3, 2019
@gopherbot gopherbot added the Proposal label Aug 3, 2019
@andybons
Copy link
Member

@andybons andybons commented Aug 6, 2019

@andybons
Copy link
Member

@andybons andybons commented Aug 6, 2019

@Ma124 you mention what you want to do, but don’t explain why you or others would need this functionality. Could you elaborate a bit more on that in your proposal? What problem does this solve?

@Ma124
Copy link
Author

@Ma124 Ma124 commented Aug 9, 2019

The Stringer would be useful for all kinds of logging purposes and I personally see no drawback.

The maxHash would be useful every time you need to show something non-go the options there are for hashing. So for example every time a user can choose a hashing algorithm in a CLI or GUI or maybe you list available hashing options to a program written in another language. And your duplicating code every time when you use the alternative: importing the package and putting it into a map. So if for example one of the choices becomes insecure you need to remove the import and the map entry and if you forget one of them you've got either an unused import which cannot be detected by the compiler or you've got a panic.

So I think there are no reasons against Stringer and very few drawbacks of a renamed maxHash.

@FiloSottile FiloSottile changed the title proposal: crypto: crypto.Hash implement fmt.Stringer and export crypto.maxHash proposal: crypto: implement Hash.String Dec 2, 2019
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Dec 2, 2019

I'm not against implementing crypto.Hash.String for logging and the like.

Since hash functions need to be imported anyway to be available, I don't see the advantage of maxHash, and it's leaking an internal detail. Also, I don't want to encourage "pick your own hash" designs. The set of available hashes in a design should be ideally of size 1, and anyway clearly specified, not "whatever Go supports".

@rsc rsc added this to Active in Proposals Feb 12, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Feb 12, 2020

Added to proposal minutes, seems headed for likely accept.

@rsc
Copy link
Contributor

@rsc rsc commented Feb 26, 2020

Based on the discussion above, this seems like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals Feb 26, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Mar 4, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Mar 4, 2020
@rsc rsc modified the milestones: Proposal, Backlog Mar 4, 2020
@rsc rsc changed the title proposal: crypto: implement Hash.String crypto: implement Hash.String Mar 4, 2020
@FiloSottile FiloSottile added NeedsFix and removed Proposal labels Mar 4, 2020
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.15 Mar 4, 2020
@katiehockman katiehockman self-assigned this Mar 20, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 23, 2020

Change https://golang.org/cl/224937 mentions this issue: crypto: implement Hash.String

@gopherbot gopherbot closed this in 9dcd6b3 Mar 24, 2020
@toothrot
Copy link
Contributor

@toothrot toothrot commented Jun 9, 2020

@katiehockman Hello! It looks like the documentation is missing for this new API: https://tip.golang.org/pkg/crypto/#Hash.String

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jun 9, 2020

@toothrot Is it common to document trivial String() string methods?

@katiehockman
Copy link
Member

@katiehockman katiehockman commented Jun 10, 2020

I just did a quick look through several crypto and x/crypto packages, and it differs (sometimes even within the same package).

From what I'm seeing, String() string methods are documented if it does something special or if more explanation is needed, e.g. https://pkg.go.dev/crypto/x509/pkix?tab=doc#Name.String and https://pkg.go.dev/golang.org/x/crypto/ssh?tab=doc#RejectionReason.String.

It isn't documented if its self explanatory, e.g. https://pkg.go.dev/crypto/x509?tab=doc#PublicKeyAlgorithm.String and https://pkg.go.dev/golang.org/x/crypto/ssh?tab=doc#RejectionReason.String.

In this case, I think String() is self-explantory. The Hash type is already described: "Hash identifies a cryptographic hash function that is implemented in another package" and any additional documentation for String() would be redundant.

@toothrot
Copy link
Contributor

@toothrot toothrot commented Jun 10, 2020

Right, sorry for the false alarm. I was merely auditing all new APIs for #39489, and following up on ones that did not have documentation.

@katiehockman
Copy link
Member

@katiehockman katiehockman commented Jun 10, 2020

No problem! Thanks for your diligence.

@ianlancetaylor ianlancetaylor mentioned this issue Jun 12, 2020
133 of 133 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.