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

x/tools/cmd/godoc: enable links to fields of structs #16753

Closed
bradfitz opened this issue Aug 16, 2016 · 30 comments
Closed

x/tools/cmd/godoc: enable links to fields of structs #16753

bradfitz opened this issue Aug 16, 2016 · 30 comments
Labels
FrozenDueToAge help wanted NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@bradfitz
Copy link
Contributor

I would like to be able to link to:

https://golang.org/pkg/net/http/#Request.Cancel

And have the link go to the top of the comment immediately before the "Cancel" field in the Request struct.

Similarly, I would like to link the English docs at the bottom of Transport, after the fields:

https://golang.org/pkg/net/http/#Transport.doc

And have it go to...

"Transport is an implementation of RoundTripper ..."

Currently my only alternative links for both are quite some distance from the text I'd like people to read.

/cc @broady @adg @shurcooL

@bradfitz bradfitz added this to the Go1.8 milestone Aug 16, 2016
@bradfitz
Copy link
Contributor Author

/cc @odeke-em too

@broady
Copy link
Member

broady commented Aug 16, 2016

The Transport.doc one is pretty bad. I don't really like the solution. Maybe the doc should be above it, anyway? I'm unsure why godoc had it swapped (compared to how it is in source) in the first place.

@bradfitz
Copy link
Contributor Author

Yes. Perhaps. There is also #16728 open for a very similar case.

@dmitshur
Copy link
Contributor

Since you didn't mention it, are you aware of the existing related behavior of godoc.org (gddo project)?

https://godoc.org/net/http#Request.Cancel

This looks like a request to backport that, plus tweaks to where on the page the anchors position you.

@dmitshur
Copy link
Contributor

Maybe the doc should be above it, anyway? I'm unsure why godoc had it swapped (compared to how it is in source) in the first place.

That's a very good question and one I've been pondering about for some months now. I'd love to know some insight on that from anyone familiar, but it might be outside of scope of this issue.

@luigi-riefolo
Copy link
Contributor

Is anyone already working on this issue/feat?

@bradfitz
Copy link
Contributor Author

bradfitz commented Sep 6, 2016

Nobody has said so here. Feel free to work on it.

@appleby
Copy link
Contributor

appleby commented Sep 6, 2016

Hi @luigi-riefolo. I looked into this briefly last week, but have no
plans to submit a changelist. Here are a few observations that might
save you some time in case you are new to the code, as I am:

  • For adding the link to doc section, have a look at
    x/tools/godoc/static/package.html.
  • In order for changes to package.html to take effect, you will need
    to re-generate the file static.go in the same directory, either by
    running go generate or else by running the makestatic command
    (located in the same directory).
  • You might consider renaming the #Transport.doc link to something
    like #Transport-doc. The reason being that there is a small chance
    of a name collision if the struct has a field named doc and the
    client passes the ?m=all query parameter, which tells godoc to
    also show unexported identifiers. This occurs, for example, in the namedType struct of the go/doc pkg.
  • As for adding links to the struct fields, a good place to start is
    the file x/tools/godoc/linkify.go. Specifically the functions
    LinkifyText
    and
    linksFor.
    These functions are responsible for (among other things) adding
    anchor links for const and var declarations in the current
    implementation.

Hope this is helpful.

@luigi-riefolo
Copy link
Contributor

Hi @appleby, thank you for the info, very useful!
I've started working on the links to the struct fields, here's a simple test:
page.html.zip

Have a look at file:///page.html#Test, file:///page.html#Test.What, file:///page.html#Test.Str.
Simply adding an id to the comment span seems enough.

I'll have a look at x/tools/godoc/linkify.go and identify a strategy to automatically add the id.

Please let me know if you guys have any comments or questions.

@appleby
Copy link
Contributor

appleby commented Sep 6, 2016

If you've figured out a way to add the id to the comment span, you should probably just go that route and ignore what I said about LinkifyText. LinkifyText is currently only responsible for adding links that anchor directly to an identifier. For example, when you link to https://golang.org/pkg/net/http/#ErrContentLength, it's anchored at the identifier, not the associated comment block.

I agree that adding the anchor at the top of the comment block is nicer from the user's perspective, but it wasn't clear to me how to get the context required to generate the id at the time the comments are rendered.

One problem with the approach of adding an id attribute to the first comment span tag is that it doesn't scale when multiple fields share the same doc comment. Consider, for example:

type Foo struct {
    // T1, T2, and T3 are templates.
    T1, T2, T3 *text.Template
}

In a case like this, if you go the route of adding an id attribute to the comment tag, you'd have to pick one of the identifiers to make linkable. Alternatively, you could possibly generate separate tags for the anchors prior to writing the comment block.

Another edge-case to keep in mind is fields without an associated doc comment. For example, see the first few fields of the godoc.Presentation struct.

@luigi-riefolo
Copy link
Contributor

@appleby great examples. I'll keep them in mind.
If you can find more of those then please let us know.

@luigi-riefolo
Copy link
Contributor

luigi-riefolo commented Sep 15, 2016

Here's the proposal only for the adding bookmarks to IDENTs (i.e. structs, vars, etc.):
It's my first proposal ever, so any comment or advice is really appreciated.

  1. In LinkifyText (linkify.go) call BookmarkText located in a new file bookmark.go:
bookmarksWriter := BookmarkText(text)
idents := tokenSelection(text, token.IDENT)
comments := tokenSelection(text, token.COMMENT)
FormatSelections(w, text, linkWriter, idents, selectionTag, bookmarksWriter, comments)

Here's its signature:
func BookmarkText(text []byte) (bw BookmarkWriter)

  1. BookmarkText identifies all the IDENTs using a helper function:

func createBookmarks(text []byte) (bookmarkMap map[int]bookmarkList)
createBookmarks parses the source code and identifies every IDENT that needs a bookmark.
If the first line of a comment is found above its relative IDENT, then a bookmark struct is pushed into a map using the comment line number.
e.g.:

      // Comment
      n := 1

If there isn't a comment block above the IDENT, or the comment is on the right-side of the IDENT, then the IDENT line number is used as key.
e.g.:
n := 1 // Comment

  1. At this point in:
    func FormatSelections(w io.Writer, text []byte, lw LinkWriter, links Selection, sw SegmentWriter, bw BookmarkWriter, selections ...Selection)

each time a token is processed a call to:
bw(w, lastOffs, start)
is made. The BookmarkWriter function checks if a lastOffs key exists in the bookmarkMap and prints its related bookmark to w.

Here's a test page:
page.html.zip

Try:

file:///page.html#ErrBodyNotAllowed
file:///page.html#SingleBlock.T3
file:///page.html#Test.What

Note: the test page has each main comment above the its relative node as pointed in #16728. To achieve this changes have been made in package.html, such as:

<h2 id="pkg-constants">Constants</h2>
    {{range .}}
        {{comment_html .Doc}}
        <pre>{{node_html $ .Decl true}}</pre>
    {{end}}

Each {{comment_html .Doc}} has been moved above <pre>{{node_html $ .Decl true}}</pre>.

Please let me know if I need to file a formal proposal or this is sufficient.

/cc @bradfitz @broady @shurcooL @appleby @adg

@dmitshur
Copy link
Contributor

dmitshur commented Sep 16, 2016

Here's a test page:
page.html.zip

That page had a Uncaught SyntaxError: Unexpected token ; error, and it wasn't being interpreted as UTF-8. I've fixed the JS error and added <meta charset="utf-8">.

I've uploaded it here for easier viewing:

http://instantshare.virtivia.com:27080/1h5htyea6hnw4.html
http://instantshare.virtivia.com:27080/1h5htyea6hnw4.html#ErrBodyNotAllowed
http://instantshare.virtivia.com:27080/1h5htyea6hnw4.html#SingleBlock.T3
http://instantshare.virtivia.com:27080/1h5htyea6hnw4.html#Test.What

I've only looked briefly, but what I can see so far, this looks very reasonable to me.

@luigi-riefolo
Copy link
Contributor

This is my latest test page before uploading the code for review.

Please have a look at some of these examples:

file:///page.html#NestedStuct.Nest1
file:///page.html#Request.Proto
file:///page.html#Err2
file:///page.html#File.io.Reader

and let me know if I missed something or there's any error.

Thanks.

/cc @bradfitz @broady @shurcooL @appleby @adg

@dmitshur
Copy link
Contributor

dmitshur commented Sep 18, 2016

I don't see any issues.

At first I thought #File.io.Reader looked questionable, and considered if it can be just #File.Reader. I wasn't sure if the package name where io.Reader comes from there was needed.

It's not needed for structs, because you can't do:

type Some struct {
    io.Reader
    foo.Reader
}

// Build error: duplicate field Reader

However, it's valid for interfaces, as long as the embedded interfaces contain non-repeating methods. So, this is possible:

type Some interface {
    io.Reader
    foo.Reader
}

// No build error (as long as there are no duplicate methods in those 2 interfaces)

So, unfortunately, it seems that it's needed to keep the package name io there.

@appleby
Copy link
Contributor

appleby commented Sep 18, 2016

I also noticed a minor thing about the #File example.

In the current docs, both the package and the type are linked separately. For example, the io.Reader in

type File interface {
    io.Closer
    io.Reader
    io.Seeker
    Readdir(count int) ([]os.FileInfo, error)
    Stat() (os.FileInfo, error)
}

"io" links to pkg io and "Reader" links to io.Reader.

https://golang.org/pkg/net/http/#File

In your example, the link to the package has been dropped, though honestly I'm not sure how useful it is to have that link to the package when you most likely to want to go straight to io.Reader anyway...

The case of nested structs is something I considered when I looked into this as well. My conclusion was that I would punt and only add links for the first-level fields of a named type. My reasoning was that nested structs are probably rare (didn't verify this), and you can get into nasty edge cases (admittedly contrived) like the following:

type Nested struct {
    Nest1, Nest2 struct {
        X int
    }
}

Would X in the above example get labeled as both Nested.Nest1.X and Nested.Nest2.X?

What about (god forbid):

type Nested struct {
    Nest1, Nest2 struct {
        Nest3, Nest4 struct {
            X int
        }
    }
}

How is X labeled in this case?

To be clear, I'm not advocating that you actually need to handle such unlikely cases. Just throwing it out there for consideration.

@appleby
Copy link
Contributor

appleby commented Sep 18, 2016

Thinking about this a bit more, I guess you can safely ignore my nested structs examples from my last comment. The point I was trying to make was that nested struct fields don't really have a canonical name and therefore it might not make sense to link to them, but I guess there is no real problem with a field having multiple ways to link to it. Especially since you are unlikely to encounter multiple levels of nesting in any real example.

@luigi-riefolo
Copy link
Contributor

@appleby, @shurcooL thank you for the feedback, it was very helpful.

In your example, the link to the package has been dropped

That's because I did not import all the requested packages in my source example. In this case the io package.

In this new test page, a struct or interface like:

type Nested struct {
    Nest1, Nest2 struct {
        X int
    }
}

will have two bookmarks respectively for Nest1 and Nest2:
Nested.Nest1, Nested.Nest2 ;
and two bookmarks for X:
Nested.Nest1.X, Nested.Nest2.X.

We limit the nesting up to 2 levels, as higher levels would be a very bad practice, thus the struct:

type Nested struct {
    Nest1, Nest2 struct {
        Nest3, Nest4 struct {
            X int
        }
    }
}

will only have bookmarks up to Nest4, excluding X because it has a nesting level of 3.

Please let me know what you think or if you want to make any changes.

@appleby
Copy link
Contributor

appleby commented Sep 26, 2016

For what it's worth, the latest test page LGTM. However, I am not a maintainer and not someone you need to convince. Sorry if that was not clear from my previous comments.

Nice work getting the links to point to the associated comment blocks. Seems like a usability win for const/var declarations as well.

@quentinmit quentinmit added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 10, 2016
@rsc
Copy link
Contributor

rsc commented Oct 10, 2016

I guess the decision here is whether to the godoc changes in the file mock are OK? That's probably up to @alandonovan or @griesemer.

@alandonovan
Copy link
Contributor

In @luigi-riefolo's file mock from Sep 25, the empty spans with ids preceding each struct field doc comment look good to me. However, the page seems to lack all the functions and methods of the HTTP package. Were these removed for expedience?

@luigi-riefolo
Copy link
Contributor

@alandonovan that is just a test page that includes non-existing data structures and part of the HTTP Request. I've tried to include as many examples as possible. Let me know if you have any additional ones.

@alandonovan
Copy link
Contributor

Thanks Luigi. All of the fragment IDs look good, with the following exception.

I disagree with Dmitri about the need for links such as #File.io.Closer. Despite their similar syntaxes, there's a crucial difference between embedding in structs and in interfaces. In a struct, an embedded field type T struct { pkg.X } is both a use of a type pkg.X and a declaration of a field X, and it is the second of these two facts that warrants a fragment #T.X. In contrast, an embedded interface type I interface { pkg.X } is still a use of the type pkg.X but it is not a declaration of anything; it doesn't introduce any new names. I think Godoc should define URLs only for things that have names in Go.

@luigi-riefolo
Copy link
Contributor

Thank you Alan for the feedback.
If anyone else agree then I'll remove the tag from the inner IDENTs within interfaces.

If you have any additional comments/questions, then please let me know.

Note: the code for bookmarking IDENTs is ready and I'm just polishing it for the peer review, but I need some more time as I'm very busy job hunting.

@dmitshur
Copy link
Contributor

dmitshur commented Oct 15, 2016

I agree with what Alan said above. That rationale is very good.

I disagree with Dmitri about the need for links such as #File.io.Closer.

My comment was not about the need for that link, it was about the need for io component if that link were to be included. I think you have described in a good way why the whole thing should not be linked. That's an even better solution.

@gopherbot
Copy link

CL https://golang.org/cl/32153 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/32154 mentions this issue.

@luigi-riefolo
Copy link
Contributor

I've just committed the changes (5 mod files, 1 new file) and don't why got two different revision number 32153 and 32154. Does anyone know why?
Furthermore I couldn't push some refs:

# git push origin HEAD:refs/for/master
Counting objects: 9, done.
Delta compression using up to 2 threads.
Compressing objects: 100% (9/9), done.
Writing objects: 100% (9/9), 1.52 KiB | 0 bytes/s, done.
Total 9 (delta 8), reused 0 (delta 0)
remote: Resolving deltas: 100% (8/8)
remote: Processing changes: new: 1, done    
remote: 
remote: New Changes:
remote:   https://go-review.googlesource.com/32154 x/tools/cmd/godoc: add links to fields
remote: 
To https://go.googlesource.com/tools
 * [new branch]      HEAD -> refs/for/master
Counting objects: 16868, done.
Delta compression using up to 2 threads.
Compressing objects: 100% (5268/5268), done.
Writing objects: 100% (16868/16868), 7.65 MiB | 89.00 KiB/s, done.
Total 16868 (delta 11370), reused 16734 (delta 11255)
remote: Resolving deltas: 100% (11370/11370)
remote: Counting objects: 16718, done
remote: Processing changes: refs: 1, done    
To https://go-review.googlesource.com/go
 ! [remote rejected] HEAD -> refs/for/master (no common ancestry)
error: failed to push some refs to 'https://go-review.googlesource.com/go'

Any ideas?

@rsc rsc modified the milestones: Go1.9, Go1.8 Nov 11, 2016
@bradfitz bradfitz self-assigned this Nov 29, 2016
@gopherbot
Copy link

CL https://golang.org/cl/33690 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/33755 mentions this issue.

gopherbot pushed a commit to golang/tools that referenced this issue Dec 1, 2016
Now without regexps and allocations.

And also match comments like:

    // Foo, if non-nil, ...

The comma confused the old pattern.

Updates golang/go#16753

Change-Id: I9016ee7b5933ea343950a39989952804c74a598b
Reviewed-on: https://go-review.googlesource.com/33755
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
@golang golang locked and limited conversation to collaborators Dec 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

9 participants