x/pkgsite: clarify requirements on package names and imports for runnable examples

[ross-spencer]

What is the URL of the page with the issue?

https://pkg.go.dev/github.com/ocfl-archive/error/pkg/error#pkg-examples

What is your user agent?

Chrome

Screenshot

What did you do?

Created an example test for the package error.

What did you see happen?

Conflict in pkg.go.dev with the type error. Locally, the tests pass, and there's no conflict with the error type.

What did you expect to see?

Local and pkg.go.dev capabilities align, either with the error being caught locally in the go compiler, or the package running as expected in the go doc examples.

[gabyhelp]

Related Issues

• x/pkgsite: Examples on golang.org/x/exp/xerrors are broken #52109 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[seankhliao]

Naming a package that shadows a builtin type just seems like a bad idea? it's essentially unusable in most go code without renaming the import.

[ross-spencer]

Naming a package that shadows a builtin type just seems like a bad idea? it's essentially unusable in most go code without renaming the import.

Absolutely! We'll take a look at renaming it but we're using a compiled language and not an interpreted one. I don't think the solution is "don't do that". Code does compile, and tests do pass. Ideally it fails sooner, or continues to pass later.

[ross-spencer]

NB. I haven't tried this @seankhliao but I assume this issue name could reflect the issue when shadowing any builtin type?

[ianlancetaylor]

Your example defines a main package, but that main package doesn't import your package. If you add the import, then I think your example will work.

[ross-spencer]

Your example defines a main package, but that main package doesn't import your package. If you add the import, then I think your example will work.

I will give that a whirl, thanks Ian.

Do you agree though that the issue is more about the divergence between the promise of local testing of examples, and when they arrive in the go pkg docs? (either a website feature, or again, something that should be improved locally?)

[ianlancetaylor]

I'm not sure I entirely understand, but I don't think I agree. Your example as written doesn't quite make sense, because it doesn't import the package that it is testing. Note that unlike other tests, examples, should not be in the same package as the code that they are showing examples for.

[ross-spencer]

unlike other tests, examples, should not be in the same package as the code that they are showing examples for.

I used this blog as reference https://go.dev/blog/examples and followed examples to the sort package. What am I missing @ianlancetaylor? do you have any references or guides that support your assertion?

[seankhliao]

As I understand, go/doc would extract examples and mutate them into complete main programs when put into files without other examples (ref go/doc.playExample). As a working example, from go-json-experiment, the examples declared in package json_test are rendered as package main.

go/doc skips resolution of all predeclared identifiers here https://go.googlesource.com/go/+/caee788a48f19814bd778c1bd2422cb6f60ad810/src/go/doc/example.go#197
Personally, I don't think we should change anything, and discourage shadowing predeclared identifiers.

[ross-spencer]

I don't think we should change anything, and discourage shadowing predeclared identifiers.

From a Ux perspective, removing run from the docs for non-compliant examples would at least be something, no?

discourage shadowing predeclared identifiers.

This doesn't feel like a sustainable strategy for a growing ecosystem and increasing number of learners. The idea of discouraging without tooling is quite folksy. Again, we're working in a compiled language here not one with duck-typing. Go.mod has a strategy to warn about ambiguities, could that be implemented for examples and docs?

Without more insight into the strategy of the go team, as an outsider, there's a workflow promise here, you can write your examples, they will test locally, and run in the docs. Again, it's more Ux than anything, but it should be clear as early as possible something won't work when pushed. And so, from an dev ergonomics perspective, I would anticipate early warning.

[seankhliao]

It should have been fairly obvious in any non trivial use of the package that it had an unsuitable name (e.g. you cannot declare a function that returns an error and imports the package without renaming at the same time).

There has never been a promise that locally running examples will run in the docs, the playground is not a full environment, not everything can be runnable, see #9679.

[ross-spencer]

Thanks for the info on #9679

It should have been fairly obvious in any non trivial use of the package that it had an unsuitable name

I'm not here to beef @seankhliao. I think we can agree to disagree on this. I believe I've mentioned some fairly fundamental reasons I naively expect this to work or receive more feedback, trivial or otherwise.

There has never been a promise that locally running examples will run in the docs

By promise, I mean it's the interface/de-facto functionality given by the language to the user, my prrimary reference: https://go.dev/blog/examples. If there are better sources folks are referring to, then I appreciate the links.

But again, I'm not here to beef. In fact, it already seems like there's clarity in this thread that can be brought to some part of the ecosystem in some way.

[ianlancetaylor]

@ross-spencer Sometimes things that seem clear to the maintainers are less clear to people less familiar with the tooling. Since examples should be usable, they should refer to names exported by the package as packagebeingtested.Name. And that means that they should import that package.

Would you like to send a patch to the testing package, and perhaps the blog entry, to make this clearer? Thanks.

[ross-spencer]

Thanks @ianlancetaylor I can endeavor to take a look!

[AndrewGMorgan]

Is the fix to delete the code line pointed to in #70611 (comment) ?

That is, this for loop? https://go.googlesource.com/go/+/caee788a48f19814bd778c1bd2422cb6f60ad810/src/go/doc/example.go#196

I think that's what @ianlancetaylor is suggesting here: #70630 (comment)

[seankhliao]

that loop needs to move after the loop that checks imports (line 252)

[findleyr]

Retitled based on my cursory understanding. IIUC, we need clearer documentation of the requirements for runnable examples. If so, a contribution from someone with fresh eyes would be helpful here.

[AndrewGMorgan]

I felt #70630 (comment) was suggesting this was actually a bug and not a documentation thing.

[gopherbot]

Change https://go.dev/cl/639175 mentions this issue: go/doc: resolve imports before predeclared identifiers in examples

[AndrewGMorgan]

Is there an ETA for this resolved bug commit to be included in the live go.dev site?

[AndrewGMorgan]

It looks to be deployed now. Thanks!