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 · 6 comments
Open

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

pjebs opened this issue Jun 24, 2020 · 6 comments

Comments

@pjebs
Copy link
Contributor

@pjebs 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 this to the Backlog milestone Jun 24, 2020
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Jun 24, 2020

/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
Member

@dmitshur 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 go101 commented Jun 26, 2020

This is a design issue which 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 was marked as resolved.

@julieqiu julieqiu closed this Aug 20, 2020
@julieqiu julieqiu reopened this Aug 20, 2020
@julieqiu

This comment was marked as resolved.

@julieqiu julieqiu modified the milestones: Backlog, pkgsite/dochtml Aug 20, 2020
@julieqiu julieqiu added the pkgsite label 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

@tooolbox tooolbox commented Aug 21, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.