Skip to content

go/doc: apply usual quote changes to package synopses #27759

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.

Sign up for GitHub

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

Jump to bottom
Closed
dotaheor opened this issue Sep 19, 2018 · 14 comments
Closed

go/doc: apply usual quote changes to package synopses #27759

dotaheor opened this issue Sep 19, 2018 · 14 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.12

Comments

@dotaheor
Copy link

A typo of "foo bar"? Or other special reasons?

https://www.google.com/search?q=site%3Agolang.org+"``"

Might be related to: #23389
@gopherbot gopherbot added the Documentation Issues describing a change to documentation. label Sep 19, 2018
@dsnet
Copy link
Member

dsnet commented Sep 19, 2018

There's magical handling in the go/doc package to convert them into “ and ”.

go/src/go/doc/comment.go

Lines 22 to 43 in 620bd5a

// Escape comment text for HTML. If nice is set,
// also turn `` into “ and '' into ”.
func commentEscape(w io.Writer, text string, nice bool) {
last := 0
if nice {
for i := 0; i < len(text)-1; i++ {
ch := text[i]
if ch == text[i+1] && (ch == '`' || ch == '\'') {
template.HTMLEscape(w, []byte(text[last:i]))
last = i + 2
switch ch {
case '`':
w.Write(ldquo)
case '\'':
w.Write(rdquo)
}
i++ // loop will add one more
}
}
}
template.HTMLEscape(w, []byte(text[last:]))
}

@dotaheor
Copy link
Author

dotaheor commented Sep 19, 2018 •
edited
Loading

but not, at least in some docs. Bug?

@dominikh
Copy link
Member

What @dotaheor is saying that these quotes aren't being replaced in synopses.

@dsnet
Copy link
Member

dsnet commented Sep 19, 2018

Ah, that wasn't clear to me. It seems to me that the commentEscape function is not being applied to the synopsis. However, I don't think it should either, since it assumes HTML is end-target output.

Personally, I'd rather see the special handling for the double back-ticks and double single-quote be removed. Go supports unicode, so the rare uses of them could just use the actual unicode characters for them.

@bcmills
Copy link
Contributor

bcmills commented Sep 22, 2018 •
edited
Loading

I'd rather see the special handling for the double back-ticks and double single-quote be removed.

I'd be happy for us to stop [ab]using them in this way in standard library sources, but note that actually removing that support from doc would be a backward-incompatible change.

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 22, 2018
@bcmills
Copy link
Contributor

bcmills commented Sep 22, 2018

CC @griesemer

@bcmills bcmills changed the title doc: there are many ``foo bar'' (two quote chars at each side) in all kinds of go docs go/doc: unescaped quotes in package synopses Sep 22, 2018
@bcmills bcmills added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 22, 2018
@bcmills bcmills added this to the Go1.12 milestone Sep 22, 2018
@rsc
Copy link
Contributor

rsc commented Sep 26, 2018

No, we're not going to remove this handling. Existing docs exist.

@rsc rsc closed this as completed Sep 26, 2018
@rsc rsc reopened this Sep 26, 2018
@rsc
Copy link
Contributor

rsc commented Sep 26, 2018

Sorry, misunderstood - OK, then we should fix the synopses to do the same replacements. But let's not redefine the doc comment syntax and invalidate existing comments.

@rsc rsc changed the title go/doc: unescaped quotes in package synopses go/doc: apply usual quote changes to package synopses Sep 26, 2018
@rsc rsc added NeedsFix The path to resolution is known, but the work has not been done. and removed Documentation Issues describing a change to documentation. labels Sep 26, 2018
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 26, 2018
@dsnet
Copy link
Member

dsnet commented Sep 26, 2018 •
edited
Loading

I'm wary of this. The replacement logic is done by doc.ToHTML and converts the quotes to &ldquo; and &rdquo;. On the other hand doc.ToText does no replacement whatsoever. The doc.Synopsis function is used for both outputting to HTML and text. Auto-converting to the HTML entity is a bad for pure text outputs.

Now, if we consistently convert the double single-quotes to the unicode equivalent in all three cases, that seems more appropriate.

@agnivade
Copy link
Contributor

I agree. I have a simpler solution if you may. We can handle this in the template render function inside godoc. With just an additional helper.

Currently, it is this {{html .Synopsis}}

We can just port the minimal logic from go/doc and have a new helper. Thus making it
{{html .Synopsis | commentEscape}}

So doc.Synopsis remains unchanged, and we still get to convert the quotes. Albeit with some very minimal code duplication.

@bcmills
Copy link
Contributor

bcmills commented Sep 27, 2018

Probably we should factor out the nice part from the HTMLEscape part: we can convert quotes in the first pass (for Synopsis and ToText), and then HTMLEscape the result.

@agnivade
Copy link
Contributor

@bcmills - Could you clarify your comment a bit ? I was looking to get going with this and wasn't fully sure what the consensus was. Did you mean to say we should convert the quotes for all the 3 functions (ToHTML, ToText and Synopsis) ? HTMLEscapeing after converting quotes will also convert the &ldquote; to &amp;ldquote;.

I believe the primary concern here is that Synopsis is used for text and html output both, so converting to html entities will be incorrect for programs expecting text output.

Or did you mean that we should convert to the unicode character directly, for all 3 cases ? That seems to be a clean solution to me.

@bcmills
Copy link
Contributor

bcmills commented Nov 16, 2018

Did you mean to say we should convert the quotes for all the 3 functions (ToHTML, ToText and Synopsis) ?

Yes: we should convert the quotes to Unicode characters (“ and ”), and if ToHTML wants to convert them further then it can transform “ to &ldquote; and ” to &rdquote;.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/150377 mentions this issue: go/doc: convert to unicode quotes for ToText and Synopsis

@golang golang locked and limited conversation to collaborators Nov 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
7 participants
@dominikh @rsc @agnivade @bcmills @dsnet @gopherbot @dotaheor