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: examples not rendered according to the testing documentation; includes unwanted code #43658

Closed
kortschak opened this issue Jan 12, 2021 · 10 comments
Labels
NeedsFix pkgsite/dochtml pkgsite

Comments

@kortschak
Copy link
Contributor

@kortschak kortschak commented Jan 12, 2021

What is the URL of the page with the issue?

https://pkg.go.dev/gonum.org/v1/gonum/graph/community#example-Profile-Simple

What is your user agent?

Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:84.0) Gecko/20100101 Firefox/84.0

Screenshot

What did you do?

Navigate to the linked example.

What did you expect to see?

According to the testing documentation

The entire test file is presented as the example when it contains a single example function, at least one other function, type, variable, or constant declaration, and no test or benchmark functions.

I expect to see just the body of the example as rendered on godoc.org since there is more than one example in the file.

// Profile calls Modularize which implements the Louvain modularization algorithm.
// Since this is a randomized algorithm we use a defined random source to ensure
// consistency between test runs. In practice, results will not differ greatly
// between runs with different PRNG seeds.
src := rand.NewSource(1)

// Create dumbell graph:
//
//  0       4
//  |\     /|
//  | 2 - 3 |
//  |/     \|
//  1       5
//
g := simple.NewUndirectedGraph()
for u, e := range smallDumbell {
    for v := range e {
        g.SetEdge(simple.Edge{F: simple.Node(u), T: simple.Node(v)})
    }
}

// Get the profile of internal node weight for resolutions
// between 0.1 and 10 using logarithmic bisection.
p, err := community.Profile(
    community.ModularScore(g, community.Weight, 10, src),
    true, 1e-3, 0.1, 10,
)
if err != nil {
    log.Fatal(err)
}

// Print out each step with communities ordered.
for _, d := range p {
    comm := d.Communities()
    for _, c := range comm {
        sort.Sort(ordered.ByID(c))
    }
    sort.Sort(ordered.BySliceIDs(comm))
    fmt.Printf("Low:%.2v High:%.2v Score:%v Communities:%v Q=%.3v\n",
        d.Low, d.High, d.Score, comm, community.Q(g, comm, d.Low))
}

What did you see instead?

Extraneous details also presented (and the imports mangled and helpful comments removed).

package main

import (
	"fmt"
	"golang.org/x/exp/rand"
	"gonum.org/v1/gonum/graph/community"
	"gonum.org/v1/gonum/graph/internal/ordered"
	"gonum.org/v1/gonum/graph/simple"
	"log"
	"sort"
)

func main() {
	// Profile calls Modularize which implements the Louvain modularization algorithm.
	// Since this is a randomized algorithm we use a defined random source to ensure
	// consistency between test runs. In practice, results will not differ greatly
	// between runs with different PRNG seeds.
	src := rand.NewSource(1)

	// Create dumbell graph:
	//
	//  0       4
	//  |\     /|
	//  | 2 - 3 |
	//  |/     \|
	//  1       5
	//
	g := simple.NewUndirectedGraph()
	for u, e := range smallDumbell {
		for v := range e {
			g.SetEdge(simple.Edge{F: simple.Node(u), T: simple.Node(v)})
		}
	}

	// Get the profile of internal node weight for resolutions
	// between 0.1 and 10 using logarithmic bisection.
	p, err := community.Profile(
		community.ModularScore(g, community.Weight, 10, src),
		true, 1e-3, 0.1, 10,
	)
	if err != nil {
		log.Fatal(err)
	}

	// Print out each step with communities ordered.
	for _, d := range p {
		comm := d.Communities()
		for _, c := range comm {
			sort.Sort(ordered.ByID(c))
		}
		sort.Sort(ordered.BySliceIDs(comm))
		fmt.Printf("Low:%.2v High:%.2v Score:%v Communities:%v Q=%.3v\n",
			d.Low, d.High, d.Score, comm, community.Q(g, comm, d.Low))
	}

}

// intset is an integer set.
type intset map[int]struct{}

func linksTo(i ...int) intset {
	if len(i) == 0 {
		return nil
	}
	s := make(intset)
	for _, v := range i {
		s[v] = struct{}{}
	}
	return s
}

var (
	smallDumbell = []intset{
		0: linksTo(1, 2),
		1: linksTo(2),
		2: linksTo(3),
		3: linksTo(4, 5),
		4: linksTo(5),
		5: nil,
	}

	middleEast = struct{ friends, complicated, enemies []intset }{

		friends: []intset{
			0:  nil,
			1:  linksTo(5, 7, 9, 12),
			2:  linksTo(11),
			3:  linksTo(4, 5, 10),
			4:  linksTo(3, 5, 10),
			5:  linksTo(1, 3, 4, 8, 10, 12),
			6:  nil,
			7:  linksTo(1, 12),
			8:  linksTo(5, 9, 11),
			9:  linksTo(1, 8, 12),
			10: linksTo(3, 4, 5),
			11: linksTo(2, 8),
			12: linksTo(1, 5, 7, 9),
		},

		complicated: []intset{
			0:  linksTo(2, 4),
			1:  linksTo(4, 8),
			2:  linksTo(0, 3, 4, 5, 8, 9),
			3:  linksTo(2, 8, 11),
			4:  linksTo(0, 1, 2, 8),
			5:  linksTo(2),
			6:  nil,
			7:  linksTo(9, 11),
			8:  linksTo(1, 2, 3, 4, 10, 12),
			9:  linksTo(2, 7, 11),
			10: linksTo(8),
			11: linksTo(3, 7, 9, 12),
			12: linksTo(8, 11),
		},

		enemies: []intset{
			0:  linksTo(1, 3, 5, 6, 7, 8, 9, 10, 11, 12),
			1:  linksTo(0, 2, 3, 6, 10, 11),
			2:  linksTo(1, 6, 7, 10, 12),
			3:  linksTo(0, 1, 6, 7, 9, 12),
			4:  linksTo(6, 7, 9, 11, 12),
			5:  linksTo(0, 6, 7, 9, 11),
			6:  linksTo(0, 1, 2, 3, 4, 5, 7, 8, 9, 10, 11, 12),
			7:  linksTo(0, 2, 3, 4, 5, 6, 8, 10),
			8:  linksTo(0, 6, 7),
			9:  linksTo(0, 3, 4, 5, 6, 10),
			10: linksTo(0, 1, 2, 6, 7, 9, 11, 12),
			11: linksTo(0, 1, 4, 5, 6, 10),
			12: linksTo(0, 2, 3, 4, 6, 10),
		},
	}
)

The parts included are used by the second example and so entirely unrelated to the first. The choice of including two examples in the file was specifically to prevent these details leaking to the reader since they do not help advance the documentation.

@gopherbot gopherbot added this to the Unreleased milestone Jan 12, 2021
@julieqiu julieqiu added the NeedsInvestigation label Jan 13, 2021
@julieqiu julieqiu removed this from the Unreleased milestone Jan 13, 2021
@julieqiu julieqiu added this to the pkgsite/unplanned milestone Jan 13, 2021
@julieqiu julieqiu removed this from the pkgsite/unplanned milestone Jan 14, 2021
@julieqiu julieqiu added this to the pkgsite/dochtml milestone Jan 14, 2021
@jba
Copy link
Contributor

@jba jba commented Jan 20, 2021

This has nothing to do with whole-file examples. It is related to our attempt to make examples playable (executable on the Go playground) by synthesizing a program from them. The code is the playExample function.

There are three bugs here:

  1. Because of the way we synthesize imports (without the benefit of a token.FileSet), they get reordered when we format. Sorting the ImportDecls ourselves doesn't help; we need to do something fundamentally different, like render the synthesized example, reparse it, and render it again. (But hopefully something less expensive than that.)

  2. When we collect dependent declarations to produce a minimal self-contained file, we do not look inside grouped declarations like the var for smallDumbell and middleEast. So we include them both.

  3. We drop some comments (the ones on middleEast in this case).

@jba jba added NeedsFix and removed NeedsInvestigation labels Jan 20, 2021
@jba jba self-assigned this Jan 20, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 22, 2021

Change https://golang.org/cl/285812 mentions this issue: internal/godoc/internal/doc: sort playable example imports

gopherbot pushed a commit to golang/pkgsite that referenced this issue Jan 22, 2021
When constructing a playable example, sort the import
declarations in the usual way, with standard library
imports first.

For golang/go#43658

Change-Id: I47008caf6c2dfa0d66351e7ffb887b0880065fcd
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/285812
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 22, 2021

Change https://golang.org/cl/285972 mentions this issue: internal/godoc/internal/doc: remove unused declarations from playable examples

@jba
Copy link
Contributor

@jba jba commented Jan 22, 2021

There is another bug: the other example in the file (https://pkg.go.dev/gonum.org/v1/gonum/graph/community#example-Profile-Multiplex) uses variables that are set up in an init function. We need to include init functions when we synthesize a playable example.

gopherbot pushed a commit to golang/pkgsite that referenced this issue Jan 25, 2021
… examples

When synthesizing the code for a playable example, we need to include
all the declared variables, types and functions that the example uses.
This CL makes sure we don't include more than we need.

If a declaration declared multiple names, like `var a, b int` or
```
var (
    a int
    b string
)
```
then we would include the entire declaration, even if the example only
used one of the variables.

Keep track the names actually used, and prune the unused ones from the
declarations.

For golang/go#43658

Change-Id: Ia9e88ecd6d4ea50bd07fad6dfba219bfcb512f0d
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/285972
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
@jba
Copy link
Contributor

@jba jba commented Feb 2, 2021

The immediate issue is fixed, so closing this. The init function bug is still outstanding. I filed #44076 for it.

@jba jba closed this as completed Feb 2, 2021
@kortschak
Copy link
Contributor Author

@kortschak kortschak commented Feb 2, 2021

If this is the solution, the documentation in the testing package describing example behaviour should be changed. It doesn't agree with this change. Or at the very least the pkgsite should say that it's forging ahead in its own direction.

@jba
Copy link
Contributor

@jba jba commented Feb 2, 2021

/cc @dmitshur I think "presented" is a case of the testing package doc overstepping its bounds.

@jba jba reopened this Feb 2, 2021
@kortschak
Copy link
Contributor Author

@kortschak kortschak commented Feb 2, 2021

Thanks

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 10, 2022

Change https://go.dev/cl/384837 mentions this issue: go/doc: group play example imports

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 22, 2022

Change https://go.dev/cl/401758 mentions this issue: go/doc: remove unused top-level declarations from playable example

gopherbot pushed a commit that referenced this issue May 10, 2022
When synthesizing a program from a playable example, preserve
the grouping of imports. That is, maintain blank lines between
imports while removing unused ones.

People are used to having those groups because that is what goimports
does.  It's disconcerting to see the all imports placed together, as
the existing code does, especially when the user has already grouped
them.

For an example, see #43658.

This is an improvement to a fix in pkgsite's fork of go/doc
(https://go.googlesource.com/pkgsite/+/7b10ef3861af4a863bf215f63b6de94c681d5af0/internal/godoc/internal/doc/example_pkgsite.go#405).
Here I've managed to avoid using a token.FileSet.

Change-Id: I65605e6dd53d742a3fe1210c3f982b54e3706198
Reviewed-on: https://go-review.googlesource.com/c/go/+/384837
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
@hyangah hyangah added the pkgsite/dochtml label May 20, 2022
@dmitshur dmitshur changed the title x/pkgsite: examples not rendered according to the testing documentation; includes unwanted code go/doc, x/pkgsite: examples not rendered according to the testing documentation; includes unwanted code May 22, 2022
@rsc rsc unassigned jba Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix pkgsite/dochtml pkgsite
Projects
None yet
Development

No branches or pull requests

5 participants