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

Closes: #5960 #5964

Merged
merged 1 commit into from
Jan 22, 2019
Merged

Closes: #5960 #5964

merged 1 commit into from
Jan 22, 2019

Conversation

jfbu
Copy link
Contributor

@jfbu jfbu commented Jan 17, 2019

@jfbu jfbu added this to the 1.8.4 milestone Jan 17, 2019
@jfbu jfbu changed the title Closes #5960 Closes: #5960 Jan 17, 2019
@codecov
Copy link

codecov bot commented Jan 17, 2019

Codecov Report

Merging #5964 into 1.8 will decrease coverage by 0.87%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              1.8    #5964      +/-   ##
==========================================
- Coverage   82.14%   81.27%   -0.88%     
==========================================
  Files         307      297      -10     
  Lines       40599    39858     -741     
  Branches     6278     5973     -305     
==========================================
- Hits        33349    32393     -956     
- Misses       5863     6042     +179     
- Partials     1387     1423      +36
Impacted Files Coverage Δ
sphinx/util/pycompat.py 47.14% <0%> (-42.86%) ⬇️
tests/test_util_inspect.py 57.19% <0%> (-33.08%) ⬇️
sphinx/util/inspect.py 49.89% <0%> (-26.57%) ⬇️
tests/test_build_htmlhelp.py 74.19% <0%> (-25.81%) ⬇️
tests/test_pycode_parser.py 91.73% <0%> (-8.27%) ⬇️
tests/test_smartquotes.py 92.5% <0%> (-7.5%) ⬇️
sphinx/ext/napoleon/__init__.py 68.67% <0%> (-7.23%) ⬇️
tests/conftest.py 85.71% <0%> (-7.15%) ⬇️
tests/test_pycode.py 94.44% <0%> (-5.56%) ⬇️
tests/test_build_texinfo.py 83.33% <0%> (-4.17%) ⬇️
... and 44 more

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 0fb0b41...2a9bd41. Read the comment docs.

@jfbu jfbu requested a review from tk0miya January 21, 2019 17:05
@nioncode
Copy link

Actually I now prefer the layout of the new parskip package, because the vertical spacing after section headings really is too much with the old one.
Are there any strong objections to bundle the new parskip with Sphinx and copying it to the latex build directory? That's what I now have in my conf.py.

The downside is that the new Sphinx release would then have a different pdf output than earlier versions.

@jfbu
Copy link
Contributor Author

jfbu commented Jan 22, 2019

@nioncode Thanks for suggestion, but no. The new parskip is used to modify core LaTeX macros, as well as core titlesec macros and an amsmath macro, modifying their behaviour in presence of core TeX skip parameter \parskip being non zero. Is this correct methodology? What if user directly does some \setlength{\parskip}{...} without loading parskip package? Then the (now considered) "misbehaviour" of original macros shows. Is parskip a core LaTeX package, is it part of the core or base LaTeX distribution? Shouldn't bugs be corrected upstream? Definitely, a novel package was needed, but not modifying behaviour of old package which had remained exactly the same since 1989.

@jfbu
Copy link
Contributor Author

jfbu commented Jan 22, 2019

Thanks for reviewing, merging.

@jfbu jfbu merged commit a11a742 into sphinx-doc:1.8 Jan 22, 2019
@nioncode
Copy link

I don't really know what's right or wrong in LaTeX terms, but to me it seems like the new parskip really has the right behavior now, because previously spacing after section headers were really big (which can be seen by having many (sub) sections that have only 1-2 lines; the spacing then is just weird).

My understanding of \parskip was that one should never modify this manually anyway, because it breaks in weird ways and that's what the parskip package is actually for.

There should at least be a way to NOT use the old revision in Sphinx. I think the only way after your merge would be to have a copy of parskip.sty in the build folder that ignores the old revision parameter. Is this the suggested usage from your point of view?

@jfbu
Copy link
Contributor Author

jfbu commented Jan 22, 2019

@nioncode The new parskip is not usable on old systems due to this

\NeedsTeXFormat{LaTeX2e}[2018-04-01]

line at its start. But assuming you deploy your project only on TeXLive 2018 type distributions (I don't recall offhand if initial TeXLive 2018 had LaTeX 2018-04-01, will check later), as you observed initial release did not have new parskip so you can add it your project by putting new file parskip.sty in your source repertory and add this to conf.py:

latex_elements = {
    'passoptionstopackages': r'\usepackage{parskip}',
}
latex_additional_files = ['parskip.sty']

This will override the sphinx.sty issued \usepackage{parskip}[=v1] on those (updated after September) TeXLive 2018 releases, which already have the new parskip. The abuse of 'passoptionstopackages' key is to load it early in preamble, before actual loading of sphinx.sty itself.

@nioncode
Copy link

Awesome, thanks for being so responsive and for fixing the original issue so quickly!

@jfbu
Copy link
Contributor Author

jfbu commented Jan 22, 2019

If you want to use the new parskip on old systems, it should work by applying my previous comment but with a custom parskip.sty modified as indicated in this diff:

diff --git a/parskip.sty b/parskip.sty
index 5e3b3db..c0b9f33 100644
--- a/parskip.sty
+++ b/parskip.sty
@@ -36,11 +36,14 @@
 %%   (C) Copyright 1989 H.Partl, TU Wien
 %%   (C) Copyright 2001 Robin Fairbairns
 %%   (C) Copyright 2018-2019 Frank Mittelbach
-\NeedsTeXFormat{LaTeX2e}[2018-04-01]
+%%
+%% HACKED FOR PERSONAL USE
+\NeedsTeXFormat{LaTeX2e}%%[2018-04-01]
 
-\DeclareRelease       {v1}{2001-04-09}{parskip-2001-04-09.sty}
-\DeclareCurrentRelease{v2}{2018-08-24}
-\ProvidesPackage{parskip}[2019-01-16 v2.0c non-zero parskip adjustments]
+%%\DeclareRelease       {v1}{2001-04-09}{parskip-2001-04-09.sty}
+%%\DeclareCurrentRelease{v2}{2018-08-24}
+%%\ProvidesPackage{parskip}[2019-01-16 v2.0c non-zero parskip adjustments]
+\ProvidesPackage{parskip}[2019/01/16 v2.0c non-zero parskip adjustments]
 \RequirePackage{kvoptions}
 \SetupKeyvalOptions{family=parskip,prefix=parskip@}
 \DeclareStringOption[0pt]{indent}[\parindent]

(untested).

I neither recommend nor endorse this and maybe legally you will need to remove some more of the comments in the file.

Edit: this indeed works on old systems. And it works on new (2018/04/01) LaTeX too, whether or not they have the new parskip installed.

@jfbu jfbu deleted the latex_parskip branch January 29, 2019 08:06
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 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.

3 participants