From 77f76ed09cb07a090ba9054063a7c002a35bca4e Mon Sep 17 00:00:00 2001 From: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com> Date: Wed, 3 Mar 2021 10:52:04 -0500 Subject: [PATCH] feat(internal/godocfx): keep some cross links on same domain (#3767) The magic of this PR (that took me way too long to figure out) comes from adding `| packages.NeedDeps` to the `packages.Load` `Config`. That lets us get the `Fset` and `Syntax` of all packages the current package imports. We can then use that information to pull out the anchors for every element we link to. This also fixes an issue where we were handling some packages that didn't have the Module field set -- we can ignore them. It appears to be a new package gets returned with the ID of the glob you asked for. For example, `cloud.google.com/go/...`. We don't seem to need that. Fixes #3739. --- internal/godocfx/godocfx_test.go | 30 ++++++++ internal/godocfx/parse.go | 79 ++++++++++++++++------ internal/godocfx/testdata/golden/index.yml | 3 +- 3 files changed, 90 insertions(+), 22 deletions(-) diff --git a/internal/godocfx/godocfx_test.go b/internal/godocfx/godocfx_test.go index 97f199bc987..17e9c72b2d4 100644 --- a/internal/godocfx/godocfx_test.go +++ b/internal/godocfx/godocfx_test.go @@ -168,3 +168,33 @@ func TestGoldens(t *testing.T) { } } } + +func TestHasPrefix(t *testing.T) { + tests := []struct { + s string + prefixes []string + want bool + }{ + { + s: "abc", + prefixes: []string{"1", "a"}, + want: true, + }, + { + s: "abc", + prefixes: []string{"1"}, + want: false, + }, + { + s: "abc", + prefixes: []string{"1", "2"}, + want: false, + }, + } + + for _, test := range tests { + if got := hasPrefix(test.s, test.prefixes); got != test.want { + t.Errorf("hasPrefix(%q, %q) got %v, want %v", test.s, test.prefixes, got, test.want) + } + } +} diff --git a/internal/godocfx/parse.go b/internal/godocfx/parse.go index 848aa959fa7..68ce87ef924 100644 --- a/internal/godocfx/parse.go +++ b/internal/godocfx/parse.go @@ -315,74 +315,93 @@ type linker struct { // different files. imports map[string]string - // idToAnchor is a map from an ID to the anchor URL for that ID. - idToAnchor map[string]string + // idToAnchor is a map from package path to a map from ID to the anchor for + // that ID. + idToAnchor map[string]map[string]string + + // sameDomainModules is a map from package path to module for every imported + // package that should cross link on the same domain. + sameDomainModules map[string]*packages.Module } func newLinker(pi pkgInfo) *linker { + sameDomainPrefixes := []string{"cloud.google.com/go"} + imports := map[string]string{} + sameDomainModules := map[string]*packages.Module{} + idToAnchor := map[string]map[string]string{} + for path, pkg := range pi.pkg.Imports { name := pkg.Name if rename := pi.importRenames[path]; rename != "" { name = rename } imports[name] = path + + // TODO: Consider documenting internal packages so we don't have to link + // out. + if pkg.Module != nil && hasPrefix(pkg.PkgPath, sameDomainPrefixes) && !strings.Contains(pkg.PkgPath, "internal") { + sameDomainModules[path] = pkg.Module + + docPkg, _ := doc.NewFromFiles(pkg.Fset, pkg.Syntax, path) + idToAnchor[path] = buildIDToAnchor(docPkg) + } } - idToAnchor := buildIDToAnchor(pi) + idToAnchor[""] = buildIDToAnchor(pi.doc) - return &linker{imports: imports, idToAnchor: idToAnchor} + return &linker{imports: imports, idToAnchor: idToAnchor, sameDomainModules: sameDomainModules} } // nonWordRegex is based on // https://github.com/googleapis/doc-templates/blob/70eba5908e7b9aef5525d0f1f24194ae750f267e/third_party/docfx/templates/devsite/common.js#L27-L30. var nonWordRegex = regexp.MustCompile("\\W") -func buildIDToAnchor(pi pkgInfo) map[string]string { +func buildIDToAnchor(pkg *doc.Package) map[string]string { idToAnchor := map[string]string{} - idToAnchor[pi.doc.ImportPath] = pi.doc.ImportPath + idToAnchor[pkg.ImportPath] = pkg.ImportPath - for _, c := range pi.doc.Consts { + for _, c := range pkg.Consts { commaID := strings.Join(c.Names, ",") - uid := pi.doc.ImportPath + "." + commaID + uid := pkg.ImportPath + "." + commaID for _, name := range c.Names { idToAnchor[name] = uid } } - for _, v := range pi.doc.Vars { + for _, v := range pkg.Vars { commaID := strings.Join(v.Names, ",") - uid := pi.doc.ImportPath + "." + commaID + uid := pkg.ImportPath + "." + commaID for _, name := range v.Names { idToAnchor[name] = uid } } - for _, f := range pi.doc.Funcs { - uid := pi.doc.ImportPath + "." + f.Name + for _, f := range pkg.Funcs { + uid := pkg.ImportPath + "." + f.Name idToAnchor[f.Name] = uid } - for _, t := range pi.doc.Types { - uid := pi.doc.ImportPath + "." + t.Name + for _, t := range pkg.Types { + uid := pkg.ImportPath + "." + t.Name idToAnchor[t.Name] = uid for _, c := range t.Consts { commaID := strings.Join(c.Names, ",") - uid := pi.doc.ImportPath + "." + commaID + uid := pkg.ImportPath + "." + commaID for _, name := range c.Names { idToAnchor[name] = uid } } for _, v := range t.Vars { commaID := strings.Join(v.Names, ",") - uid := pi.doc.ImportPath + "." + commaID + uid := pkg.ImportPath + "." + commaID for _, name := range v.Names { idToAnchor[name] = uid } } for _, f := range t.Funcs { - uid := pi.doc.ImportPath + "." + t.Name + "." + f.Name + uid := pkg.ImportPath + "." + t.Name + "." + f.Name idToAnchor[f.Name] = uid } for _, m := range t.Methods { - uid := pi.doc.ImportPath + "." + t.Name + "." + m.Name + uid := pkg.ImportPath + "." + t.Name + "." + m.Name idToAnchor[m.Name] = uid } } @@ -436,11 +455,20 @@ func (l *linker) linkify(s string) string { // pattern. func (l *linker) toURL(pkg, name string) string { if pkg == "" { - if anchor := l.idToAnchor[name]; anchor != "" { + if anchor := l.idToAnchor[""][name]; anchor != "" { name = anchor } return fmt.Sprintf("#%s", name) } + if mod, ok := l.sameDomainModules[pkg]; ok { + pkgRemainder := pkg[len(mod.Path)+1:] // +1 to skip slash. + // Note: we always link to latest. One day, we'll link to mod.Version. + baseURL := fmt.Sprintf("/go/docs/reference/%v/latest/%v", mod.Path, pkgRemainder) + if anchor := l.idToAnchor[pkg][name]; anchor != "" { + return fmt.Sprintf("%s#%s", baseURL, anchor) + } + return baseURL + } baseURL := "https://pkg.go.dev" if name == "" { return fmt.Sprintf("%s/%s", baseURL, pkg) @@ -558,7 +586,7 @@ type pkgInfo struct { func loadPackages(glob, workingDir string) ([]pkgInfo, error) { config := &packages.Config{ - Mode: packages.NeedName | packages.NeedSyntax | packages.NeedTypes | packages.NeedTypesInfo | packages.NeedModule | packages.NeedImports, + Mode: packages.NeedName | packages.NeedSyntax | packages.NeedTypes | packages.NeedTypesInfo | packages.NeedModule | packages.NeedImports | packages.NeedDeps, Tests: true, Dir: workingDir, } @@ -594,7 +622,7 @@ func loadPackages(glob, workingDir string) ([]pkgInfo, error) { } if strings.Contains(id, "_test") { id = id[0:strings.Index(id, "_test [")] - } else { + } else if pkg.Module != nil { idToPkg[pkg.PkgPath] = pkg pkgNames = append(pkgNames, pkg.PkgPath) // The test package doesn't have Module set. @@ -674,3 +702,12 @@ func loadPackages(glob, workingDir string) ([]pkgInfo, error) { return result, nil } + +func hasPrefix(s string, prefixes []string) bool { + for _, prefix := range prefixes { + if strings.HasPrefix(s, prefix) { + return true + } + } + return false +} diff --git a/internal/godocfx/testdata/golden/index.yml b/internal/godocfx/testdata/golden/index.yml index 045f4026600..d27f1b544d0 100644 --- a/internal/godocfx/testdata/golden/index.yml +++ b/internal/godocfx/testdata/golden/index.yml @@ -793,7 +793,8 @@ items: - go syntax: content: func (b *BucketHandle) - IAM() *iam.Handle + IAM() *iam.Handle - uid: cloud.google.com/go/storage.BucketHandle.If name: | func (*BucketHandle) If