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.

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

Do not escape hover content #658

Merged
merged 2 commits into from
Jul 20, 2019
Merged

Do not escape hover content #658

merged 2 commits into from
Jul 20, 2019

Conversation

rwols
Copy link
Member

@rwols rwols commented Jul 19, 2019 •
edited

Close #657.

Otherwise mdpopups will complain.
Also add two hover tests to cement this behavior.

cc @LoneBoco

@coveralls
Copy link

coveralls commented Jul 19, 2019 •
edited

Coverage Status

Coverage remained the same at 38.038% when pulling 5acab78 on bugfix/escape-hover-text into 1e09409 on master.

@rwols rwols closed this Jul 19, 2019
@rwols rwols reopened this Jul 19, 2019
@jfcherng
Copy link
Contributor

jfcherng commented Jul 20, 2019 •
edited

image

PHP seems not to be fixed by this PR.

@rwols
Copy link
Member Author

rwols commented Jul 20, 2019

You're right. I also noticed in clangd that template parameters are escaped too many times now. Let us revert this small change and think of another solution.

@rwols rwols changed the title Do not escape quote characters for hover content Do not escape hover content Jul 20, 2019
@LoneBoco
Copy link
Contributor

Is the problem the fact that some things are returning HTML entities and by escaping, we turn < into <? I've been reading some of the Markdown specs some more (there is no one Markdown spec at this time, which makes everything really complicated), and it seems that it is probably safest to ensure that the LSP servers themselves are encoding all angle brackets and ampersands. It is probably best to just rollback #650 and I can work with the Omnisharp team to end up with something that works well with LSP plugins.

@rwols rwols requested a review from tomv564 July 20, 2019 20:35
@rwols rwols merged commit eba6b07 into master Jul 20, 2019
@rwols rwols deleted the bugfix/escape-hover-text branch July 20, 2019 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomv564 tomv564 tomv564 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

hover regression
5 participants
@rwols @coveralls @jfcherng @LoneBoco @tomv564