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

Hotfixes new tutorial #9339

Merged
merged 6 commits into from
Jun 14, 2021
Merged

Hotfixes new tutorial #9339

merged 6 commits into from
Jun 14, 2021

Conversation

astrojuanlu
Copy link
Contributor

@astrojuanlu astrojuanlu commented Jun 14, 2021

Feature or Bugfix

  • Bugfix

Purpose

  • Fix PDF generation by replacing non-ASCII character
  • Fix accidental rewrap

Relates

Rendered version: https://sphinx--9339.org.readthedocs.build/en/9339/tutorial/index.html

@astrojuanlu
Copy link
Contributor Author

@jfbu By the way, I tried make latexpdf for the Sphinx docs and it didn't work even on the 4.0.x branch, where the new tutorial is not present. I guess there is some sort of special procedure to do it, or perhaps I'm missing some TeXLive dependency.

Comment on lines +99 to +107
docs
├── build
├── make.bat
├── Makefile
└── source
├── conf.py
├── index.rst
├── _static
└── _templates
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you feel about adding / to make it clearer what's a directory and what's not?

Suggested change
docs
├── build
├── make.bat
├── Makefile
└── source
├── conf.py
├── index.rst
├── _static
└── _templates
docs/
├── build/
├── make.bat
├── Makefile
└── source/
├── conf.py
├── index.rst
├── _static/
└── _templates/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the unmodified output of tree, see #9276 (comment)

I don't have strong feelings either way, but I'm pinging @stevepiercy because he reviewed this part in the first PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the unmodified output of tree because it reduces maintenance and the names of directories do not include /.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... I was wondering why my tree has that by default, and I found that it's because it's aliased to tree -F.

I disagree with some stuff said here but that's water under the bridge now and there's better stuff to spend our collective energy on than trailing slashes! :)

Copy link
Contributor

@jfbu jfbu left a comment

Choose a reason for hiding this comment

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

LGTM with nits

Also the paragraph above the code-block seems to have suffered some reflowing. Isn't it supposed to be displayed as a list?

@jfbu
Copy link
Contributor

jfbu commented Jun 14, 2021

I tried make latexpdf for the Sphinx docs and it didn't work even on the 4.0.x branch,

It compiles at my locale. I can't tell more about your problem without seeing the error...

@astrojuanlu
Copy link
Contributor Author

@jfbu No worries, I'll report the error separately then.

About your review, I think you didn't submit your nitpicks? Also, what paragraph do you refer to?

@jfbu
Copy link
Contributor

jfbu commented Jun 14, 2021

oh sorry my nitpicks were only the paragraph issue i.e. https://github.com/sphinx-doc/sphinx/pull/9276/files#r651195319

@astrojuanlu astrojuanlu requested a review from jfbu June 14, 2021 19:39
Copy link
Contributor

@jfbu jfbu left a comment

Choose a reason for hiding this comment

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

LGTM with nits, there is another copy of the offending Unicode character on line 44

Thanks for contribution!

@jfbu
Copy link
Contributor

jfbu commented Jun 14, 2021

@jfbu No worries, I'll report the error separately then.

I think you branch does not integrate the #9333 fix currently merged in 4.0.x, so make latexpdf fails. But this does not matter, I tested at my locale merging it additionally, to check and it compiled.

@jfbu
Copy link
Contributor

jfbu commented Jun 14, 2021

ah no wait there is yet another reflowed list-into-paragraph near the end

This showcases several features of the reStructuredText syntax, including:

- a **section header** using ``===`` for the underline, - two examples of
  :ref:`rst-inline-markup`: ``**strong emphasis**`` (typically bold) and
  ``*emphasis*`` (typically italics), - an **inline external link**, - and a
  ``note`` **admonition** (one of the available :ref:`directives
  <rst-directives>`)

@jfbu
Copy link
Contributor

jfbu commented Jun 14, 2021

On other tidbit is that this syntax:

``build/``

  An empty directory (for now) that will hold the rendered documentation.

``make.bat`` and ``Makefile``

  Convenience scripts to simplify some common Sphinx operations, such as
  rendering the content.

etc...

is rendered using quoting for each item. It is not a description list due to blank lines. Suppressing the blank lines like this

``build/``
  An empty directory (for now) that will hold the rendered documentation.

``make.bat`` and ``Makefile``
  Convenience scripts to simplify some common Sphinx operations, such as
  rendering the content.

etc

I get this in PDF

Capture d’écran 2021-06-14 à 22 37 34

and in html

Capture d’écran 2021-06-14 à 22 39 16

but with the white lines I get this (still in html)

Capture d’écran 2021-06-14 à 22 40 24

notice extra whitespace.

Please remove the extra blank lines.

@astrojuanlu
Copy link
Contributor Author

Damn, I'm not having a brilliant day today 🤦🏽 thanks for your patience @jfbu. Going through these fixes.

@jfbu
Copy link
Contributor

jfbu commented Jun 14, 2021

Damn, I'm not having a brilliant day today 🤦🏽 thanks for your patience @jfbu. Going through these fixes.

don't worry, I did some sloppy reviewing and should have avoided torturing you piecemeal :)

@astrojuanlu
Copy link
Contributor Author

Done, thanks again @jfbu :)

Copy link
Contributor

@jfbu jfbu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

I will merge after testing completes

@jfbu jfbu merged commit 7507989 into sphinx-doc:4.x Jun 14, 2021
@astrojuanlu astrojuanlu deleted the hotfix-new-tutorial branch June 14, 2021 20:59
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants