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

html/template: HTMLEscapeString duplicates functionality found in package html #20957

Closed
mappu opened this issue Jul 9, 2017 · 3 comments
Closed
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@mappu
Copy link

mappu commented Jul 9, 2017

This is a very minor issue.

I noticed today that html/template.HTMLEscapeString() and html.EscapeString() do have separate, independent implementations.

Why?

Is it possible (or even desirable) to unify them?

EDIT: The former has some additional small optimizations that do not appear in the latter: (1) avoid allocation by early bailout; and (2) avoid creating package-global strings.Replacer var.

@bradfitz
Copy link
Contributor

bradfitz commented Jul 9, 2017

It's also in text/template.

The package-global strings.Replacer you found does do the 'avoid allocation by early bailout' already. So that's not a reason.

The bigger difference is one escapes the NUL byte and the other doesn't.

If we were to de-duplicate this, we'd want to make the heavy packages (html/template and text/template) depend on the smaller package (html). The html/template already depends on html. text/template doesn't, which would mean a new dep, which might be okay, except even the signature it needs is different from what it has currently. One is based on strings and one is based on []byte and io.Writer.

We can keep this open to track, but like you said: minor.

@bradfitz bradfitz added this to the Go1.10 milestone Jul 9, 2017
@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 9, 2017
@bradfitz
Copy link
Contributor

bradfitz commented Jul 9, 2017

I'll mark this NeedsDecision. The decision can be made once we see how invasive a fix might be.

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017
@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 29, 2018
@rsc
Copy link
Contributor

rsc commented Nov 14, 2018

This doesn't really matter.

@rsc rsc closed this as completed Nov 14, 2018
@golang golang locked and limited conversation to collaborators Nov 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge 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

5 participants