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

x/tools/cmd/godoc: don't show internal values #30370

Open
perillo opened this issue Feb 23, 2019 · 9 comments
Open

x/tools/cmd/godoc: don't show internal values #30370

perillo opened this issue Feb 23, 2019 · 9 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@perillo
Copy link
Contributor

perillo commented Feb 23, 2019

I'm not sure if this is a duplicate of #11758.

I have noted a workaround in the Go Firestore client implementation to avoid Godoc showing the interval value:
https://github.com/googleapis/google-cloud-go/blob/master/firestore/options.go#L34

I think this is undesiderable. I have noted the problem with other package documentations, e.g. https://godoc.org/golang.org/x/text/encoding/charmap.

Isn't it better if Godoc don't show interval values of global variables?

Thanks.

@gopherbot gopherbot added this to the Unreleased milestone Feb 23, 2019
@dsnet
Copy link
Member

dsnet commented Feb 24, 2019

In some cases you do want to show that information. If you hide it, then it provides no way for that information to be intentionally shown. In some cases you don't want to show that information, in which case the workaround is as straight forward: #11758 (comment).

Either policy results is not ideal for one use-case or the other. However, the current policy is more flexible, so seems like the better choice.

@perillo
Copy link
Contributor Author

perillo commented Feb 24, 2019

It is true that it is straight forward, but the solution suggested by Russ will still show something that, IMHO, is not useful for people who read the documentation, and may show some internal details.

The variable documentation in the package https://godoc.org/golang.org/x/text/encoding/charmap is an example.

Godoc, shows:

var CodePage037 *Charmap = &codePage037

that just add noise.

What about a custom convention to make godoc hide the initialization?
As an example, using the _ prefix:

var X = _x

Thanks.

@agnivade
Copy link
Contributor

We already have a lot of conventions like BUG and DEPRECATED. I personally would like to keep godoc minimal and clean. In general, adding _ to variables or method receivers is not considered idiomatic Go. Writing comments in a certain way is fine, but godoc should not dictate how one should write the code

/cc @dmitshur @andybons for decision.

@agnivade agnivade added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 24, 2019
@perillo
Copy link
Contributor Author

perillo commented Feb 24, 2019

Keeping godoc simple is a good reason.

But I still don't like the fact that one should modify the code to make a tool do the right thing, as with https://github.com/googleapis/google-cloud-go/blob/master/firestore/options.go#L34. But probably this is a very rare case.

Thanks

@dsnet
Copy link
Member

dsnet commented Feb 25, 2019

make a tool do the right thing

With something like semi-auto-generated documentation which is consumed by humans, it's not always clear what "the right thing" is. As mentioned earlier, there are cases where you do want the unexported initializers to be shown and some cases where you don't. In your suggested convention, how would users who do want the unexported initializer to be shown achieve that?

var X = _x

I'm not sure I agree that is a better documented. It doesn't tell me the type of X at all.

solution suggested by Russ will still show something that

The solution by Russ is one of many. If you want absolutely nothing shown, you could do:

var ExportedGlobal MyType

func init() {
    ExportedGlobal = ... // hidden variable initialization logic
}

@bcmills
Copy link
Contributor

bcmills commented Feb 25, 2019

Note that the case in #11758 is somewhat different: in that example, the initializer itself doesn't include any explicit unexported identifiers (or even function calls), so it's less obvious that the initializer is not useful to external users.

@bcmills
Copy link
Contributor

bcmills commented Feb 25, 2019

If you hide it, then it provides no way for that information to be intentionally shown.

What are some examples where you intentionally want to show explicit unexported identifiers in public documentation?

Also note that we have an existing mechanism for explicitly showing unexported parts of an API: the ?m=all query parameter to the godoc server and the -u flag to go doc.

@dsnet
Copy link
Member

dsnet commented Feb 25, 2019

What are some examples where you intentionally want to show explicit unexported identifiers in public documentation?

Here are some real examples (names mangled for obvious reasons):

var MaxSize = 1024 * 1024 * envUint64("MAX_LOG_MB")

var MaxTimeout = Duration(100*milliseconds)

var GlobalCache = &Cache{maxSize: 64*1024*1024}

@bcmills
Copy link
Contributor

bcmills commented Feb 25, 2019

var MaxSize = 1024 * 1024 * envUint64("MAX_LOG_MB")

Ok, that one is kinda neat. But how is the user supposed to know what envUint64 does? (Is the number an integer, a floating-point value, or something else? If it's set but invalid, does the program panic, or ignore it? Are there special distinguished values?)

So that's arguably clearer using an explicit comment, or using only exported functions:

// MaxSize is the maximum size, in bytes, of a log file.
//
// If the MAX_LOG_MB environment variable is a valid integer,
// MaxSize is initialized from it;
// otherwise, MaxSize is initialized to to DefaultMaxSize.
var MaxSize

or

var MaxSize = 1024 * 1024 * env.Uint64("MAX_LOG_MB")

(where the user can then follow a link to view the documentation for env.Uint64 in some helper-package.)

var MaxTimeout = Duration(100*milliseconds)

Why would the user prefer that over an exported standard-library type with proper documentation?

var MaxTimeout = 100 * time.Millisecond
var GlobalCache = &Cache{maxSize: 64*1024*1024}

If maxSize is user-configurable, why not use the standard configuration hook instead?

var GlobalCache = NewSIzedCache(64 * 1024 * 1024)

or

var GlobalCache = &Cache{MaxSize: 64 * 1024 * 1024}

If it isn't user-configurable, why does the user care about its configuration? There isn't anything they can do to change it anyway.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

5 participants