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

do not escapeHTML() characters that are not HTML special characters #437

Closed
wants to merge 1 commit into from
Closed

do not escapeHTML() characters that are not HTML special characters #437

wants to merge 1 commit into from

Conversation

Mithgol
Copy link
Contributor

@Mithgol Mithgol commented Jul 3, 2015

The method escapeHTML() is said to escape HTML special characters.

However, it also escapes ¢, £, ¥, , ©, ®. These characters do not have any special meaning in HTML and thus their conversion (in my opinion) does not bring any real benefits but only unnecessarily lengthens a string and wastes processing power.

This pull request prevents their further escaping by partially reverting #417. The issue #301 remains fixed because the unescapeHTML() method is not altered.

@stoeffel
Copy link
Collaborator

stoeffel commented Jul 3, 2015

I actuality don't think this hurts and can see a little benefit for users. I even consider adding some other characters (i.e. ü, ä, ö). @epeli what do you think about this?

@Mithgol
Copy link
Contributor Author

Mithgol commented Jul 3, 2015

can see a little benefit for users

What's the use case?

@stoeffel
Copy link
Collaborator

stoeffel commented Jul 4, 2015

What's the use case?

I don't have a use case at hand 😳. Maybe @srmelody who wrote #417 has a use case.

But still, I don't see a down side to the current implementation and I don't think it's a big performance issue.

@megawac
Copy link
Contributor

megawac commented Jul 4, 2015

Underscore's and lodash's escape function only handle the HTML entities required. However, on second thought, most of this may be better placed in a speciality library like https://github.com/mathiasbynens/he

Mithgol added a commit to Mithgol/node-fidonet-fidohtml that referenced this pull request Jul 4, 2015
Mithgol added a commit to Mithgol/phido that referenced this pull request Jul 8, 2015
@Mithgol
Copy link
Contributor Author

Mithgol commented Feb 27, 2016

There weren't any further discussion in seven months and thus I can conclude that the patch is unlikely to be accepted.

This pull request is now closed, I worked around it by using .escape() from Underscore as @megawac recommended.

@Mithgol Mithgol closed this Feb 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants