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/website, x/pkgsite, x/tools/cmd/godoc: context.Context.Value isn't present in index, and comment on ordering of functions that return a type #37674

Open
coreyog opened this issue Mar 4, 2020 · 6 comments
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. pkgsite/dochtml Issues related to package documentation in pkgsite pkgsite

Comments

@coreyog
Copy link

coreyog commented Mar 4, 2020

Recently I've been working with a developer that's new to go and is using the documentation to get up to speed. There have been a couple things missing from the index of some documentation pages.

For example, https://golang.org/pkg/context has no documentation around (context) Value(key interface{}) interface{}. It's shown in an example and mentioned in the interface under type Context but the function feels too important not to be listed in the index or have its own paragraph detailing it.

There's a similar situation with Dial(network string, address string) (net.Conn, error) in the net package. It's talked about in the overview but missing from the index.

@gopherbot gopherbot added this to the Unreleased milestone Mar 4, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Mar 5, 2020

It's true that the Context.Value method is not in the index of the context package documentation. The index only lists the Context interface. We should investigate if this can or should be improved.

I don't see where this applies in the net package. There is a Dial function in the index. It's listed under type Conn, because it's considered a "constructor" so to speak, given it returns (Conn, error):

The Dialer.Dial method is also in the index:

Can you clarify if I'm misunderstanding your point about the net package?

@dmitshur dmitshur added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Mar 5, 2020
@coreyog
Copy link
Author

coreyog commented Mar 5, 2020

To me, package level functions shouldn't be indented under anything. I know it doesn't show a receiver for it but my first read is that there's a type called Conn and it has a function called Dial. Also, I understand and agree that it works like a constructor but in a language without formal constructors I wouldn't expect the documentation to be organized like it is a constructor.

@dmitshur
Copy link
Contributor

dmitshur commented Mar 5, 2020

Thanks for your feedback.

Putting functions that return a specific type under that type is a feature that has existed for many years now. To make progress here, need to investigate what were the trade-offs considered when choosing to implement this feature.

Another factor is that many Go users are used to the current behavior, so a change would be costly. Any potential benefits of a change need to be weighed against the cost of doing it.

There may be some existing work related to the design of the index on pkg.go.dev (/cc @julieqiu) which can take this issue into account.

Labeling as NeedsInvestigation.

/cc @griesemer @julieqiu

@dmitshur dmitshur changed the title x/website: Incomplete documentation? x/website, go.dev, x/tools/cmd/godoc: context.Context.Value isn't present in index, and comment on ordering of functions that return a type Mar 5, 2020
@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Mar 5, 2020
@coreyog
Copy link
Author

coreyog commented Mar 5, 2020

The logic behind net.Dial makes sense eventually, and I believe new go people can figure that out and run with it. But when it comes to context.Value(...), I'm less sure what the reasoning is.

@dmitshur
Copy link
Contributor

dmitshur commented Mar 5, 2020

I agree that not including the Context.Value method of the context.Context interface is likely worth spending more time on.

It's worth taking into account that the context package is not very representative of most Go packages. It's very small, and has an interface with many methods that include detailed and nuanced implementation.

Compare it to a package like net/http or io, which has many interfaces that are small and their individual methods are less important. For example, the index for net/http has the Handler interface:

Including its ServeHTTP method in the index may not be as helpful in contrast to the other functions associated with that type.

Similarly, the http.Client struct has many fields, but they may or may not be as helpful to list as its methods.

@julieqiu julieqiu modified the milestones: Unreleased, pkgsite/dochtml Aug 21, 2020
@julieqiu julieqiu changed the title x/website, go.dev, x/tools/cmd/godoc: context.Context.Value isn't present in index, and comment on ordering of functions that return a type x/website, x/pkgsite, x/tools/cmd/godoc: context.Context.Value isn't present in index, and comment on ordering of functions that return a type Aug 21, 2020
@tooolbox
Copy link

tooolbox commented Aug 21, 2020

Putting functions that return a specific type under that type is a feature that has existed for many years now. To make progress here, need to investigate what were the trade-offs considered when choosing to implement this feature.

Another factor is that many Go users are used to the current behavior, so a change would be costly. Any potential benefits of a change need to be weighed against the cost of doing it.

Agreed. As soon as you grok that the "constructor" methods are top of the list under a type in the Index, it becomes very valuable. You look at a type, you can see how to make one and how to use it. I would venture that any intention to change this bears the burden of proof that Gophers at large would find this beneficial.

@hyangah hyangah added the pkgsite/dochtml Issues related to package documentation in pkgsite label May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. pkgsite/dochtml Issues related to package documentation in pkgsite pkgsite
Projects
None yet
Development

No branches or pull requests

6 participants