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

rst format fix, ~~~ was being used instead of --- under subsections c… #939

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

daniel-demelo
Copy link

…ausing the document not to render properly

@dbader
Copy link
Member

dbader commented Nov 25, 2018

Thanks for the PR @daniel-demelo. The headline hierarchy seems incorrect:

screenshot 2018-11-25 10 54 23

The goal is to have a h1 section title and then have the other sub-headings nested correctly underneath that, 1-2 levels deep.

@@ -3,7 +3,7 @@
Pipenv & Virtual Environments
=============================

.. image:: /_static/photos/35294660055_42c02b2316_k_d.jpg
.. image:: ../_static/photos/35294660055_42c02b2316_k_d.jpg
Copy link
Member

Choose a reason for hiding this comment

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

I believe this will break the image when deployed to python-guide.org.

Copy link
Author

Choose a reason for hiding this comment

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

without it is broken for sure.
You can checkout the previous commit before I added this modification, the image does not appear at all.
Also, on GitHub help section about readme files (https://help.github.com/articles/about-readmes/) says:
"Relative links are easier for users who clone your repository. Absolute links may not work in clones of your repository - we recommend using relative links to refer to other files within your repository."
I am able to see everything at the moment on GitHub with the proper image rendered.

Copy link
Author

Choose a reason for hiding this comment

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

I just built it locally with Sphinx and it worked fine.
No broken image.

Copy link
Member

Choose a reason for hiding this comment

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

So what I'm trying to say here is that the main target for this is the python-guide.org website, we've had this discussion on other PRs and for technical reasons we need the image URLs to stay the way they are until we revamp our deployment process for the website. Please remove the .. prefix, thanks!

@daniel-demelo
Copy link
Author

Ok, I guess I can also fix the headline hierarchy.
I'll add another commit to this PR to fix that.

I just took a look at The Guide Style Guide and it says that for "sub section headings" you should use a tilde line as below:

Very Deep
~~~~~~~~~

Now, the very reason that the page rendering was not working, was exactly because the tilde line was breaking the document, so either I am very wrong here, or this should be reviewed.
Here's the link for a Style guide for Sphinx-based documentations. Take a look at the headings section, there are no tilde lines for headers formatting.

The Guide Style Guide might need to be updated

@dbader
Copy link
Member

dbader commented Nov 25, 2018

Now, the very reason that the page rendering was not working, was exactly because the tilde line was breaking the document, so either I am very wrong here, or this should be reviewed.
Here's the link for a Style guide for Sphinx-based documentations. Take a look at the headings section, there are no tilde lines for headers formatting.

The Guide Style Guide might need to be updated

Thanks for bringing it up, looks like we should revisit that too :-)

Basically the goal here is to have a nice and SEO-friendly header structure where each page has a single h1 and then everything else in that chapter nested underneath it.

If you're interested in giving it a shot and putting together a PR to refresh the style guide, that would make my day! /cc @mpoulin

@daniel-demelo
Copy link
Author

I'll do PR for the style guide later.
I noticed that in the style guide there's no mention about indentation (how many spaces it should be, if tabs are allowed, ..)
There was differing indentation sizes on virtualenvs.rst, most of the document seemed to use 4 spaces tabs, so I went along with it. If you want spaces only let me know.

@dbader
Copy link
Member

dbader commented Nov 26, 2018

Thanks, this is starting to look good—let's go with 4 spaces for consistency.

screenshot 2018-11-26 13 22 45

^ Does it make sense to have autoenv nested under virtualenv-burrito? To me they seem like two separate things

@daniel-demelo
Copy link
Author

Done :)

@mpoulin
Copy link
Contributor

mpoulin commented Dec 6, 2018

@daniel-demelo @dbader

I have a pull request #945 that fixes this ~~~~~~ problem. Sphinx started throwing warnings when I fixed the H1/H2 headers.

I'll wait for you to merge first before I do my merge.

@mpoulin
Copy link
Contributor

mpoulin commented Dec 21, 2018

Hi @daniel-demelo , @dbader

I'd like to bring @apjanke into this discussion because he is running into this same problem.

@apjanke
Copy link
Contributor

apjanke commented Dec 22, 2018

I agree with @daniel-demelo’s analysis here: those ‘~~~~~’-underlined headings, while valid RST, are not one of the levels that Sphinx recognizes for its structural headings. So the Guide Style Guide is mistaken in recommending their use, and should be updated to recommend (I think) ‘——-‘ and ‘^^^^^^’ for its deeply-nested headings.

(That would explain the issue I got: it looks like because ‘~~~~~~’ is not a special Sphinx structural level, it gets interpreted as an h1; thus the “Inconsistent heading levels” warning I saw when it was used in the place of a lower level heading.)

The changes in this PR look good to me.

@kennethreitz
Copy link
Contributor

maybe we should fix sphinx?

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

5 participants