-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Hotfixes new tutorial #9339
Conversation
@jfbu By the way, I tried |
docs | ||
├── build | ||
├── make.bat | ||
├── Makefile | ||
└── source | ||
├── conf.py | ||
├── index.rst | ||
├── _static | ||
└── _templates |
There was a problem hiding this comment.
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?
docs | |
├── build | |
├── make.bat | |
├── Makefile | |
└── source | |
├── conf.py | |
├── index.rst | |
├── _static | |
└── _templates | |
docs/ | |
├── build/ | |
├── make.bat | |
├── Makefile | |
└── source/ | |
├── conf.py | |
├── index.rst | |
├── _static/ | |
└── _templates/ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 /
.
There was a problem hiding this comment.
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! :)
There was a problem hiding this 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?
It compiles at my locale. I can't tell more about your problem without seeing the error... |
@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? |
oh sorry my nitpicks were only the paragraph issue i.e. https://github.com/sphinx-doc/sphinx/pull/9276/files#r651195319 |
There was a problem hiding this 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!
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>`) |
On other tidbit is that this syntax:
is rendered using quoting for each item. It is not a description list due to blank lines. Suppressing the blank lines like this
I get this in PDF and in html but with the white lines I get this (still in html) notice extra whitespace. Please remove the extra blank lines. |
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 :) |
Done, thanks again @jfbu :) |
There was a problem hiding this 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
Feature or Bugfix
Purpose
Relates
Rendered version: https://sphinx--9339.org.readthedocs.build/en/9339/tutorial/index.html