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

Comments

Projects
None yet
5 participants
@perillo
Copy link

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Member

commented Feb 24, 2019

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.

@perillo

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.