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

refactor chm_htmlescape() #5862

Merged
merged 1 commit into from Dec 24, 2018
Merged

Conversation

tk0miya
Copy link
Member

@tk0miya tk0miya commented Dec 24, 2018

@tk0miya tk0miya added this to the 1.8.3 milestone Dec 24, 2018

if PY2:
assert chm_htmlescape("Hello 'world'") == "Hello 'world'"
assert chm_htmlescape("Hello 'world'", True) == "Hello 'world'"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is new behavior on py2. At present, it works only for .hhc file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just reverted this. It's too much.

@codecov
Copy link

codecov bot commented Dec 24, 2018

Codecov Report

Merging #5862 into 1.8 will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              1.8    #5862      +/-   ##
==========================================
+ Coverage   82.08%   82.09%   +0.01%     
==========================================
  Files         301      307       +6     
  Lines       40160    40521     +361     
  Branches     6201     6258      +57     
==========================================
+ Hits        32964    33266     +302     
- Misses       5817     5870      +53     
- Partials     1379     1385       +6
Impacted Files Coverage Δ
sphinx/builders/htmlhelp.py 96.42% <100%> (+0.02%) ⬆️
tests/test_build_htmlhelp.py 100% <100%> (ø) ⬆️
sphinx/make_mode.py 0% <0%> (ø)
sphinx/search/__init__.py 96.9% <0%> (ø)
sphinx/quickstart.py 0% <0%> (ø)
sphinx/errors.py 80% <0%> (ø)
sphinx/__init__.py 60% <0%> (ø)
sphinx/apidoc.py 0% <0%> (ø)
sphinx/builders/html.py 82.95% <0%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f92aa7...6ffe549. Read the comment docs.

@ghost
Copy link

ghost commented Dec 24, 2018

Maybe we should also change title = chm_htmlescape(title) to title = chm_htmlescape(title, True) at line 332:
https://github.com/sphinx-doc/sphinx/pull/5862/files#diff-934a4ea68760fc7303dd44a301179b73R332

Imagine someone building a .chm using Python2, and a quote " appears in title, this seems a bug in this situation.

@tk0miya
Copy link
Member Author

tk0miya commented Dec 24, 2018

@animalize Absolutely!

f.write(item)
title = chm_htmlescape(title)
title = chm_htmlescape(title, True)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I add 2nd argument to fix the bug. see CHANGES.

@ghost
Copy link

ghost commented Dec 24, 2018

I put this htmlhelp.py into X:\Python37\Lib\site-packages\sphinx\builders, then I can build a correct python372rc1.chm.

@tk0miya
Copy link
Member Author

tk0miya commented Dec 24, 2018

Thank you for reviewing! Merging now.

@tk0miya tk0miya merged commit f8c6c14 into sphinx-doc:1.8 Dec 24, 2018
@tk0miya tk0miya deleted the refactor_chm_htmlescape branch December 24, 2018 05:17
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant