Skip to content

go/doc: make formatting comments more flexible using an interface #37868

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

Closed
wants to merge 4 commits into from

Conversation

JAicewizard
Copy link

Tools, like gopls, have to basically copy-paste go/doc/comment.go and replace certain aspects using for their own format. This means having to keeping it up-to-date with go/doc and thus a maintenance burden.

This change removes that burden for developers.

Fixes #34875

@googlebot googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Mar 15, 2020
@gopherbot
Copy link
Contributor

This PR (HEAD: cb821c4) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/223577 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

This PR (HEAD: 28c787e) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/223577 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Rebecca Stambler:

Patch Set 2:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/223577.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Jaap Aarts:

Patch Set 2:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/223577.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rebecca Stambler:

Patch Set 2:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/223577.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: f34cb68) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/223577 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Ingo Oeser:

Patch Set 3:

(1 comment)

Small suggestion on wording.


Please don’t reply on this GitHub thread. Visit golang.org/cl/223577.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 1780d4a) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/223577 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Jaap Aarts:

Patch Set 3:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/223577.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Robert Griesemer:

Patch Set 4:

Thanks for this initial outline.

Let's discuss and agree on the proposal first before you do more work on this.

Regarding this specific CL: The Formatter's API seems very specific to the current implementation as it mostly factors out some of the underlying calls into methods. If we decide to accept this proposal, I would like to see a simplified and less implementation-specific API.

Thanks.


Please don’t reply on this GitHub thread. Visit golang.org/cl/223577.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Jaap Aarts:

Patch Set 4:

Patch Set 4:

Thanks for this initial outline.

Let's discuss and agree on the proposal first before you do more work on this.

Regarding this specific CL: The Formatter's API seems very specific to the current implementation as it mostly factors out some of the underlying calls into methods. If we decide to accept this proposal, I would like to see a simplified and less implementation-specific API.

Thanks.

I would agree, this started out as just an extension to the already implemented method but it does look complex and has a lot of methods.
The official spec on comments was very non-descriptive of the contents, the other (https://blog.golang.org/godoc) was very non-specific too.
I think an interface could look like

interface{
    writeHead(text string)
    writeText(text string)
    writeUrl(text, match string, italic bool)
    writeRaw(text string)
}

The implementation would have to keep track of line wraps, and newlines at the end of paragraphs.
I haven't looked at idents yet, there is a part for that in the regex in emphasize but I don't know how it is being used. I would like to add an writeIdent(text, match string, italic bool) too.

I think this would be very simple and extendable to different formats


Please don’t reply on this GitHub thread. Visit golang.org/cl/223577.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rebecca Stambler:

Patch Set 2:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/223577.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Jaap Aarts:

Patch Set 2:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/223577.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Rebecca Stambler:

Patch Set 2:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/223577.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Ingo Oeser:

Patch Set 3:

(1 comment)

Small suggestion on wording.


Please don’t reply on this GitHub thread. Visit golang.org/cl/223577.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Jaap Aarts:

Patch Set 3:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/223577.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Robert Griesemer:

Patch Set 4:

Thanks for this initial outline.

Let's discuss and agree on the proposal first before you do more work on this.

Regarding this specific CL: The Formatter's API seems very specific to the current implementation as it mostly factors out some of the underlying calls into methods. If we decide to accept this proposal, I would like to see a simplified and less implementation-specific API.

Thanks.


Please don’t reply on this GitHub thread. Visit golang.org/cl/223577.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Jaap Aarts:

Patch Set 4:

Patch Set 4:

Thanks for this initial outline.

Let's discuss and agree on the proposal first before you do more work on this.

Regarding this specific CL: The Formatter's API seems very specific to the current implementation as it mostly factors out some of the underlying calls into methods. If we decide to accept this proposal, I would like to see a simplified and less implementation-specific API.

Thanks.

I would agree, this started out as just an extension to the already implemented method but it does look complex and has a lot of methods.
The official spec on comments was very non-descriptive of the contents, the other (https://blog.golang.org/godoc) was very non-specific too.
I think an interface could look like

interface{
    writeHead(text string)
    writeText(text string)
    writeUrl(text, match string, italic bool)
    writeRaw(text string)
}

The implementation would have to keep track of line wraps, and newlines at the end of paragraphs.
I haven't looked at idents yet, there is a part for that in the regex in emphasize but I don't know how it is being used. I would like to add an writeIdent(text, match string, italic bool) too.

I think this would be very simple and extendable to different formats


Please don’t reply on this GitHub thread. Visit golang.org/cl/223577.
After addressing review feedback, remember to publish your drafts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

go/doc: add ToMarkdown
4 participants