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

go/doc,x/pkgsite: rethink order for factory functions #39813

Open
pjebs opened this issue Jun 24, 2020 · 9 comments
Open

go/doc,x/pkgsite: rethink order for factory functions #39813

pjebs opened this issue Jun 24, 2020 · 9 comments
Labels
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

@pjebs
Copy link
Contributor

pjebs commented Jun 24, 2020

For my package: https://godoc.org/github.com/rocketlaunchr/dataframe-go/forecast

I have a function:
func Forecast(ctx context.Context, sdf interface{}, r *dataframe.Range, alg ForecastingAlgorithm, cfg interface{}, n uint, evalFunc EvaluationFunc) (interface{}, []Confidence, float64, error) which returns an interface{} as the first return value.

image

Unfortunately my function is categorized under type Confidence when it should not be.

I assume this issue would also be present in pkg.go.dev, but my package has been banned from there (despite the person in charge saying it will be allowed many many months ago), so I can't check if this issue persists.

@cagedmantis cagedmantis changed the title godoc formatting not correct godoc: formatting not correct Jun 24, 2020
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 24, 2020
@cagedmantis cagedmantis added this to the Backlog milestone Jun 24, 2020
@cagedmantis
Copy link
Contributor

/cc @dmitshur

@dmitshur dmitshur changed the title godoc: formatting not correct go/doc: associated type of factory functions may not be the first returned type Jun 25, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Jun 25, 2020

Thanks for reporting. This behavior is a part of the go/doc package, so it affects all places that use it for displaying Go documentation.

The Forecast function is being considered as a "factory function" for the type Confidence because it returns a slice of that type, and all other return value types are predeclared type.

This is working as intended given the current implementation, see the relevant code in go/doc here.

Note that this is a heuristic, so it is expected it may not produce absolutely ideal results in all cases. The heuristic was chosen with a goal of working as well as possible as often as possible. Based on your description, it sounds this is a case where it's not optimal.

We could consider changing the heuristic to require associated type of factory functions to be the first result type, rather than the first visible result type. In order to gain confidence that it would be a net positive to make such a change, we could test it out on a large corpus of Go packages and evaluate whether it results in more changes that are more helpful than unhelpful.

/cc @griesemer @julieqiu

@go101
Copy link

go101 commented Jun 26, 2020

This is a design issue which breaks user expectations and has been reported for several times: #28006 and #38666

Personally, I would like to list such functions both in the function list and the method list of a type. (BTW, I have implemented this in an experimental project.)

@julieqiu

This comment has been minimized.

@julieqiu julieqiu reopened this Aug 20, 2020
@julieqiu

This comment has been minimized.

@julieqiu julieqiu modified the milestones: Backlog, pkgsite/dochtml Aug 20, 2020
@julieqiu julieqiu changed the title go/doc: rethink order for factory functions go/doc,x/pkgsite: rethink order for factory functions Aug 20, 2020
@tooolbox
Copy link

The factory-function heuristics work great the vast majority of the time. It may be surprising initially, but once you understand the organization it's very valuable. Perhaps we need to work on the "surprising" aspect, but breaking them out from under their respective types is a grass-is-greener proposition, since we trade one use case for another and it's not clear it's an improvement.

Personally, I would like to list such functions both in the function list and the method list of a type.

I understand the sentiment, but I believe this has a couple of harmful effects:

  • It bloats the Index so the package looks bigger than it actually is.
  • A reader then needs to compare the two occurrences in the Index to be sure they're actually the same, rather than some close spelling.

My suggestion is, if your package has constructor methods that you expect to require regular usage, and you want to spotlight them, put some examples in the Overview section at the top. In #28006 you mention net/http but I have never wondered how to use that package because the first thing you see in the docs are examples of http.Get() and http.Post().

We could consider changing the heuristic to require associated type of factory functions to be the first result type, rather than the first visible result type. In order to gain confidence that it would be a net positive to make such a change, we could test it out on a large corpus of Go packages and evaluate whether it results in more changes that are more helpful than unhelpful.

This sounds like a change that could handle @pjebs specific issue without negative impacts.

@lpar
Copy link

lpar commented May 22, 2023

I've only just worked out why I keep failing to find things in the navigation on pkg.go.dev, and it's this problem. I'd been assuming the navigation is broken and using Ctrl-F.

I would like to request that at least one of the navigation sections be useful for finding any function in the package, regardless of what type(s) it returns.

This could be done by making Functions list all functions, or by arranging the index in alphabetical order by name. Both were things I expected to be the case already.

In my case, I was looking up os.Stat (the function, not the method on *File). I failed to find it under Functions, and I failed to find it in the index between Setenv and Symlink. If you use the Search packages or symbols box, the search results describe it as "function Stat in os", so it makes no sense to me that it isn't listed under functions.

@mitar
Copy link
Contributor

mitar commented Aug 3, 2023

I also have a problem with the order in my gitlab.com/tozd/go/errors package. In there I have func UnmarshalJSON(data []byte) (error, E) which should not be listed under the E type.

From my understanding, the following suggestion would help, no?

We could consider changing the heuristic to require associated type of factory functions to be the first result type

@go101
Copy link

go101 commented Aug 3, 2023

Golds avoids this problem (caused by unreasonable design) by creating two lists (As Outputs Of and As Inputs Of) under each type (an example).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

9 participants