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: add ToMarkdown #34875

Open
JAicewizard opened this issue Oct 13, 2019 · 32 comments
Open

go/doc: add ToMarkdown #34875

JAicewizard opened this issue Oct 13, 2019 · 32 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal Proposal-Accepted
Milestone

Comments

@JAicewizard
Copy link

Recently gopls added markdown output for markdown comments, the implementation is basically a copy of go/doc/comment.go replacing some things with markdown instead of html.
If go/doc was more flexible this would have been less of a copy paste. These should be a interface which can be used to create custom formatters. In this way packages that need backwards compatibility with older go stdlibs can create a formatter and use this with older versions of go. The type implementing this can be copy pasted into go/doc/comment.go(or vice versa).

I am willing to implement this together with porting the gopls markdown implementation to go/doc

@julieqiu
Copy link
Member

/cc @dmitshur

@julieqiu julieqiu added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 15, 2019
@julieqiu julieqiu added this to the Unreleased milestone Oct 15, 2019
@JAicewizard
Copy link
Author

JAicewizard commented Nov 27, 2019

Hi, although its been a while I have functional code and an interface that should work in all situations. I have implemented this interface with all the old formats and markdown.
Can somebody check if I should open a CL for it? (not s in check my code, just in general)

@stamblerre
Copy link
Contributor

@JAicewizard: We are currently in the freeze part of the Go release cycle (see https://github.com/golang/go/wiki/Go-Release-Cycle), so no new features will be accepted. I would suggest sending a CL when the tree is opened after Go 1.14 is released.

@JAicewizard
Copy link
Author

Thanks for the info, I didn't want to start with sending in a CL without any discussion on what the interface should look like, but I will set a reminder to send one in February.

@stamblerre
Copy link
Contributor

Sounds good. I think if you have a CL ready, you should be fine to send it in February and discussion can occur during the review process. Otherwise, you can mark this issue as a proposal to spark the conversation (just add the prefix "proposal: " to the issue title).

@gopherbot
Copy link

Change https://golang.org/cl/223577 mentions this issue: go/doc: make formatting comments more flexible using an interface

@dmitshur
Copy link
Contributor

Thanks for sending that change, @JAicewizard. It will be helpful to have that prototype when discussing and considering this change.

The go/doc package is low level and in the standard library. Once new API is added to it, it is not possible to change due to the Go 1 compatibility promise. As a result, any potential API additions need to be discussed and agreed on first. (See issue #23864 for an example of a previous proposal to add new functionality to the go/doc package.)

We may decide that adding this new functionality would be better to do in another package (not in the standard library). It depends on whether we can expect the proposed API to solve the problem well today and in the future, without needing further API changes. Until then, it may be less costly to just maintain copies of the code in the necessary packages.

/cc @griesemer @agnivade per owners.

@dmitshur dmitshur modified the milestones: Unreleased, Backlog Mar 18, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Mar 18, 2020

Also see CL 72890 for related work. It is an attempt to split documentation rendering into smaller and more flexible building blocks. An updated copy of that package is being used to render documentation on pkg.go.dev, which is going to be open sourced (see #36747).

@andybons andybons changed the title go/doc: rework go/doc to be more flexible proposal: go/doc: allow custom formatters via an interface Mar 18, 2020
@JAicewizard
Copy link
Author

This implementation is very close to identical to the old implementation for html (to help reduce any added maintenance cost) and allows for very flexible implementations. The implementation is basically given some string and the context around the string to format it in whatever way you would want to.
The idea here is to reduce copying the go standard library, not necessarily to bring something people are already using into the standard library.

I understand that there needs to be discussion first, that is why I opened this issue.

@agnivade
Copy link
Contributor

It's a reasonable proposal. Joe's CL is proof enough that this is needed by libraries out there. The core idea looks good to me. We can hash out the nitty gritties of the new API if the proposal is accepted.

@griesemer
Copy link
Contributor

Per my comment in the CL, I think the interface outlined in the CL is too complicated. We should have a pretty clear idea of what a realistic API looks like before accepting the proposal (not the other way around).

@JAicewizard
Copy link
Author

Do mean the interface already implemented or the interface outlined in #37868 (comment). My reasoning for the later API is that all comments are essentially a list of text blocks, with headers, urls, idents, and "raw"/code blocks in between.
This interface would give the formatter the ability to do with those blocks of text whatever it wants. This also makes parsing easy, and doesn't put any extra burden on the formatter.

@griesemer
Copy link
Contributor

Sorry for being imprecise. I think the new outlined interface looks great but I haven't spend much time thinking through it.

@JAicewizard
Copy link
Author

I too think the interface implemented in the CL isn't very good and I didn't actually put much time into thinking about it.

One of the benefits of the new interface that you might not immediately think of is being able to type-assert the formatters compatibility with a potential new feature. You can then adjust the internals to the formatters capabilities. AKA adding a new feature wouldn't have to be breaking.

@dolmen
Copy link
Contributor

dolmen commented Nov 9, 2020

The CL removes the ToText and ToHTML functions from the go/doc API.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jan 6, 2021
@rsc
Copy link
Contributor

rsc commented Jan 13, 2021

In general defining this interface is going to expose lots of details I'd really rather not commit to.
It is fine to fork and modify a package when that is appropriate, as it is here when you need internal details that aren't exposed.

It would probably make sense to add a ToMarkdown alongside ToText and ToHTML.
That would be a lot easier to design the API for than adding an interface (the API is the same as ToHTML).

@rsc
Copy link
Contributor

rsc commented Jan 13, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) Jan 13, 2021
@jimmyfrasche
Copy link
Member

An easy way to get the AST of the parsed comments would be better, imo. The unexported blocks function does most of this but maybe it should split paragraphs and headings into spans of text and quotations and urls and the like. Then any new format is a simple pretty printer. That would also make it easy to do adjacent things like extracting just the headings for building an outline.

@rsc
Copy link
Contributor

rsc commented Jan 20, 2021

@jimmyfrasche the issue with the AST interface is that if we expand the doc syntax (we have talked about adding some list form at some point, for example) then all the pretty-printers that consume AST have to learn about something new.
If we stick to the narrower interface of ToText, ToHTML, ToMarkdown, then programs using those just keep working.

@rsc
Copy link
Contributor

rsc commented Jan 20, 2021

Let's see about adding ToMarkdown. I will retitle this. The proposal is to add

func ToMarkdown(w io.Writer, text string, words map[string]string)

which has the same signature as

func ToHTML(w io.Writer, text string, words map[string]string)

Does anyone object to this?
Would gopls (the motivation in the top comment above) be able to use it?

/cc @dmitshur @julieqiu

@rsc rsc changed the title proposal: go/doc: allow custom formatters via an interface proposal: go/doc: add ToMarkdown Jan 20, 2021
@JAicewizard
Copy link
Author

@jimmyfrasche the issue with the AST interface is that if we expand the doc syntax (we have talked about adding some list form at some point, for example) then all the pretty-printers that consume AST have to learn about something new.
If we stick to the narrower interface of ToText, ToHTML, ToMarkdown, then programs using those just keep working.

I get your motivation behind this. You could argue that changing the syntax would not be go1 compatible, because a program might rely on the output of those functions to stay the same. I would say that this means the docs related code should to in golang.org/x, but that decision is long gone, before I started using go even.

If programs really want another output format, I would say parsing converting markdown and outputting something else shouldn't be a problem since there are plenty markdown parsers. This would help prevent copy-pasting of go source code.

@jimmyfrasche
Copy link
Member

There's precedent for implementing standard library packages in terms of vendored golang.org/x packages. In this case, if lists were added the golang.org/x module could be made v2 and anyone using the narrower interfaces in go/doc would be unaffected

@rsc
Copy link
Contributor

rsc commented Jan 27, 2021

Ping @dmitshur and @julieqiu to confirm that adding ToMarkdown would help gopls.
(You are the ones who were commenting at the top so I assume there is a use case.
I am not sure whether it is gopls or pkg.go.dev, but above it says gopls.)

@rsc
Copy link
Contributor

rsc commented Jan 28, 2021

I spoke to @julieqiu about this. Her memory is that she was just triaging back in Oct 2019 and thought @dmitshur was the owner (thinking of godoc not go/doc maybe). So I retract my question to both of you.

@rsc
Copy link
Contributor

rsc commented Feb 3, 2021

I'm not seeing anyone objecting to this, and given the centrality of markdown in today's dev environments, it seems worth doing.

@rsc
Copy link
Contributor

rsc commented Feb 3, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Feb 3, 2021
@dividinglimits
Copy link

I'm not seeing anyone objecting to this, and given the centrality of markdown in today's dev environments, it seems worth doing.

In my humble opinion, saying that because something is in favor "today" it should be added to the stdlib is a decision that should be examined from a different time unit. At one point, XML was in favor, in more recent time, other formats (such as Markdown) have become more in favor. I think it's reasonable to conclude then, that Markdown could suffer from the same fate.

With that said, I use Markdown frequently, and enjoy the syntax, but am skeptical of adding this export that must be supported forever (or until non 1.x becomes a reality, which may be never, given other discussions)

@bcmills
Copy link
Contributor

bcmills commented Feb 4, 2021

@dividinglimits, if folks do stop using Markdown at some point, what's the harm in keeping an unused ToMarkdown function around in the standard library? It's a simple, stable format, so once it's implemented the cost of maintenance should be quite low, and the linker can prune it out from programs that don't call it.

@dividinglimits
Copy link

I do not see harm, but it does set (or extend) a precedent that because something is in favor now, it is a good candidate for addition. My concern is about the line of demarcation fitting this profile, not whether this capability is a helpful.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Feb 10, 2021
@rsc
Copy link
Contributor

rsc commented Feb 10, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: go/doc: add ToMarkdown go/doc: add ToMarkdown Feb 10, 2021
@dsnet
Copy link
Member

dsnet commented Apr 14, 2021

As I look over the landscape of existing GoDoc features and proposed features, I'm not sure if:

ToMarkdown(w io.Writer, text string, words map[string]string)

is the API that we want to commit to. When I built the first prototype of hotlinks years ago, I pretty quickly came to conclusion that the existing ToHTML API is too limited. Furthermore, the pkgsite implementation already doesn't make use of the ToHTML function.

I agree with @jimmyfrasche that we should expose the AST in some way. If the concern is the forwards compatibility, I think there are ways to expose the API in a way that grows gracefully. If this was done, then the pkgsite could benefit from it. Right now it has a fork of the internal blocks function.

@JAicewizard
Copy link
Author

(sorry for being a bit late) Thats why I originally put out a patch to expose these blocks. Although this may be a bit too heavy of a change for the compatibility guarantee, I still think that maybe a package that exposes that API in some place like golang.org/x/doc would be usefull.

Anyways would it be ok for me to copy-paste the markdown implementation for gopls into stdlib and make submit patch with that? It might not address the wider usecase, but it will implement this approved proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal Proposal-Accepted
Projects
Status: Accepted
Development

Successfully merging a pull request may close this issue.