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

proposal: wiki: CodeReviewComments change #37495

Closed
l0k18 opened this issue Feb 27, 2020 · 3 comments
Closed

proposal: wiki: CodeReviewComments change #37495

l0k18 opened this issue Feb 27, 2020 · 3 comments

Comments

@l0k18
Copy link

l0k18 commented Feb 27, 2020

Regarding this section:

https://github.com/golang/go/wiki/CodeReviewComments#named-result-parameters

There is an extra case that I thought of recently as I was writing a lot of functions that were taking functions with more than a single parameter and return value, and it a couple of things occurred to me, related to that section there:

  • If the function signature is only used once, put good suggested names in the signature where it appears inside the signature.

  • If the function signature is used in two or more places, it is suggested to define the function signature as a type, and put the nice parameter/return value names there.

It may seem trivial but it affects both readability and programmer-user experience. Closures in Go are already enforced (for what reason, I am not yet clear on, my guess is implementation and being a bit too macro-like) to be verbosely pasted in everywhere when they are populated with a function. Every time one is written there is a big ugly signature there to look at. If the parameters (at least) are not named, then it's very noisy, you get absolutely no information from looking at it, what it does, except, if it's there, the name of the parameter that is a function.

So the two guidelines above are my recommendation for adding to this document in relation to naming function parameters (and when used a lot, naming the signature itself using a type definition), mitigate some of the noise introduced by closure function signatures, and reduce the work required to use them - one can just jump to the function, jump to its named function signature, copy, add {} and start putting some code inside there.

@andybons andybons changed the title wiki: CodeReviewComments change proposal: wiki: CodeReviewComments change Feb 27, 2020
@gopherbot gopherbot added this to the Proposal milestone Feb 27, 2020
@rsc
Copy link
Contributor

rsc commented Mar 4, 2020

There are counter-examples to this rule:

package ast: func Inspect(node Node, f func(Node) bool)
package crypto: func RegisterHash(h Hash, f func() hash.Hash)

And plenty more. Sometimes your rule makes some sense, and other times it does not.
Most often it boils down to whether the names are necessary to the documentation.
If the name is not needed in the docs, then they are just noise that don't appear anywhere else.

If anything the rule is: do what makes the code and docs clearest.
That's not always putting in names.

This seems like a likely decline, since it is contrary to standard practice as evidenced by the standard library.

@rsc
Copy link
Contributor

rsc commented Mar 4, 2020

If this gets declined, should close #37384 too.

@rsc
Copy link
Contributor

rsc commented Mar 11, 2020

No change in consensus, so declined.

@rsc rsc closed this as completed Mar 11, 2020
@golang golang locked and limited conversation to collaborators Mar 18, 2021
@rsc rsc added this to Proposals Aug 10, 2022
@rsc rsc moved this to Declined in Proposals Aug 10, 2022
@rsc rsc removed this from Proposals Oct 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants