The Wayback Machine - https://web.archive.org/web/20201215131525/https://github.com/python/devguide/pull/283
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

Use an appropriate highlight language throughout #283

Merged
merged 4 commits into from Oct 24, 2017

Conversation

@jeff5
Copy link
Contributor

@jeff5 jeff5 commented Oct 14, 2017

No description provided.

jeff5 added 2 commits Oct 14, 2017
Replace the default (Python) highlighting where a Pygments lexer is
available, and use "none" elsewhere. For console commands follow the
example of gitbootcamp.rst mostly ($-prompt and lang=console).
@jeff5
Copy link
Contributor Author

@jeff5 jeff5 commented Oct 14, 2017

As an example of what this aims to fix, consider why certain words are coloured and others italic in the examples here: https://devguide.python.org/documenting/#lists-and-quotes and (more subtly) here: https://devguide.python.org/#quick-reference .

I have taken gitbootcamp.rst as illustrating the right way to do it.

In some places (gdb, buildbots), I am of colouring-in some things I don't understand, so happy to be put right. Now fit for review.

buildbot user "when the computer starts up". It is best to provide
absolute paths to the ``buildslave`` command and the :file:`buildarea`
directory. It is also recommended to set the task to run in the
directory that contains the :file:`buildarea` directory.

* Alternatively (note: don't do both!), set up the buildslave
* Alternatively (note: don't do both!), set up the buildslave

This comment has been minimized.

@ezio-melotti

ezio-melotti Oct 15, 2017
Member

This change seems unrelated, but if you are going to fix the whole paragraph should be dedented 1 space, not just the *.

This comment has been minimized.

@jeff5

jeff5 Oct 15, 2017
Author Contributor

Except for code, indenting is supposed to be on a multiple of 3 according to https://devguide.python.org/documenting/#use-of-whitespace . The text of this list item is indented at 6, so I'm just moving the item marker from 4 to 3 as part of aligning the enclosing margin to column 3. You may notice that in a lot of console code I inserted "$" in a carefully-chosen column not "$ ".

So that's consistent, but now I look again, this enclosing indent looks spurious and everything should be 3 columns left of where I left it. Separate PR?

clang.rst Outdated
$ sudo mkdir /usr/local/bin/scan-build
$ sudo cp -r llvm-3.4/tools/clang/tools/scan-build /usr/local/bin
$ sudo mkdir /usr/local/bin/scan-view
$ sudo cp -r llvm-3.4/tools/clang/tools/scan-view /usr/local/bin

This comment has been minimized.

@ezio-melotti

ezio-melotti Oct 15, 2017
Member

Keep in mind that in many of these examples, the $ are intentionally omitted to make the commands easy to copy-paste. Does the highlight not work without the $?

This comment has been minimized.

@jeff5

jeff5 Oct 15, 2017
Author Contributor

Without the $, the console lexer treats it as output. A workable alternative, is to omit the prompt and choose the bash lexer as I did adjacently. In this case, after $, no highlighting is generated anyway so bash, none, console: it's all one!

@@ -63,57 +64,57 @@ Step-by-step Guide
You should have already :ref:`set up your system <setup>`,
:ref:`got the source code <checkout>`, and :ref:`built Python <compiling>`.

* Create a new branch in your local clone::
* Create a new branch in your local clone::

This comment has been minimized.

@ezio-melotti

ezio-melotti Oct 15, 2017
Member

What's the reason for this change?

This comment has been minimized.

@jeff5

jeff5 Oct 15, 2017
Author Contributor

Again I'm indenting on 3. And the nearby $ are indented at 6. It was hard to see the on-topic console commands correctly indented when the surrounding list were not.

This comment has been minimized.

@ezio-melotti

ezio-melotti Oct 16, 2017
Member

I think the 3-chars indentation only affects the position of the *, and the rest is aligned with the first char after the * (so 2 chars). Similarly if I have a piece of Python code, the code block is indented 3 chars, but within the block the code is indented with 4 characters.
I don't think I ever saw the text of a list indented like this.

This comment has been minimized.

@jeff5

jeff5 Oct 23, 2017
Author Contributor

I recommend trying it if your editor tabs to column 3 for .rst files. But I've reverted what I did here. I've not attempted to fix the indentation generally as part of this PR.

Copy link
Member

@terryjreedy terryjreedy left a comment

Don't add $s. This was discussed a few months ago, when it was agreed that existing one should be removed, unless there were a pair of unix and windows versions of commands. They definitely do not belong with system agnostic python commands, like the one I noted.

@@ -41,7 +43,7 @@ There are three ways of visualizing recent build results:
to display the latest build results on the development ("master") branch,
type::

bbreport.py -q 3.x
$ bbreport.py -q 3.x

This comment has been minimized.

@terryjreedy

terryjreedy Oct 15, 2017
Member

'$' is system-specific (why don't you use '>' for the benefit of Windows newbies), just noise, and a nuisance.
We should be getting rid of them where they are, not adding.

This comment has been minimized.

@jeff5

jeff5 Oct 16, 2017
Author Contributor

As I flagged, I'm taking my lead from gitbootcamp,rst. It's not covered in the style guide, except where the text is a Python console and there it is well-established that we do show the prompt. Distinguishing input and output seemed useful in some of the console material I visited. Is this an established style rule?

I use Windows and see a $ with my git commands because I use git bash. While the prompt where I run Sphinx actually says devguide PS> , I'm not phased by seeing $ universally in guides.

This comment has been minimized.

@ezio-melotti

ezio-melotti Oct 16, 2017
Member

I usually include the prompt if the output is also present in order to distinguish the two. If there is no output then I usually omit the prompt to make the commands easy to copy/paste. For Python snippets in the main docs we solved the problem by adding a button that removes the prompt/output, so the code can be copy/pasted in both cases.

This comment has been minimized.

@jeff5

jeff5 Oct 16, 2017
Author Contributor

The prompt & output stripper is good and I missed it as soon as I came to write my own docs. WIBNI lexers supported it as standard.

Ok, I'll revert chunks of this (most of the added $ signs and the indentation adjustments) and then see what highlight language choice produces the most readable effect. The indenting is non-standard in lots of places, but that's another PR, if at all. Fresh commit to follow.

This comment has been minimized.

@ezio-melotti

ezio-melotti Oct 16, 2017
Member

If you notice an highlight style that is prevalent throughout the devguide, we could set that as the default highlight so you don't have to specify it for each code block/document.

This comment has been minimized.

@jeff5

jeff5 Oct 23, 2017
Author Contributor

In case it wasn't clear, I've reverted my insertion of prompts and chosen highlight language correspondingly.

Where the $-prompt is explicit, highlight as console, and where implicit
as bash (mostly).
@jeff5
Copy link
Contributor Author

@jeff5 jeff5 commented Oct 17, 2017

I've partially reverted to satisfy review feedback (I hope). There are now no added $-prompts. I've gone with what the content author had decided before this PR, and chosen the highlight language to match. IMO it's definitely better with in some of those cases, e.g. where make echoes commands, and without in others. There was no dominant mode overall (bash:console=5:4) but often a clear winner per file.

Indenting is also as it was.

Assuming you squash-commit, please tweak the commit message: I suspect it won't quite make sense otherwise.

@Mariatta
Copy link
Member

@Mariatta Mariatta commented Oct 17, 2017

It wasn't necessary to bring that up here and calling it out as the "worst" example.

@jeff5
Copy link
Contributor Author

@jeff5 jeff5 commented Oct 18, 2017

Sorry @Mariatta . I meant only to call to mind the lesson learned (in good humour) about a pitfall in a fairly new process. If I can, I'll alter history to remove the link. (Forgive that too, please).

Copy link
Collaborator

@willingc willingc left a comment

In general, I like using making sure code blocks are being highlighted correctly. This is a big PR with a lot of text files to compare the changes side by side. I wonder if it makes more sense to make smaller PRs with less files. I'm not sure whether:

  1. highlighting the whole file and then just highlighting the individual code blocks that are different
  2. just highlighting every code block

is less error prone over time.

compile (formatting added for clarity): ::
compile (formatting added for clarity):

.. code-block:: none

This comment has been minimized.

@merwok

merwok Oct 23, 2017
Member

Why not console?

This comment has been minimized.

@jeff5

jeff5 Oct 23, 2017
Author Contributor

Because the console lexer thinks the lines beginning # in the output are commands. (Traditionally, # is the prompt in a shell with root privilege.)

There's not much in it either way, but I went for less distracting.

This comment has been minimized.

@merwok

merwok Oct 23, 2017
Member

Ah! Thanks, it is the best choice.

@jeff5
Copy link
Contributor Author

@jeff5 jeff5 commented Oct 23, 2017

@willingc wrote

This is a big PR with a lot of text files to compare the changes side by side.

Side-by-side provides a good check I haven't accidentally changed what we're saying.

In fact it's not possible to review it like that, because the highlight directive at the top of a file affects all the code blocks. documenting.rst is the outstanding example. One really has to build it and look at the generated pages affected by the change. I did this, of course, when considering (and reconsidering) the choice of per-file highlight, and for individual block exceptions (strategy 1 in your comment).

I don't favour choosing the highlight language per block generally (strategy 2), as it makes the :: shorthand impossible and adds 3 lines each time. If I have a misgiving, it is adding the occasional highlight directive elsewhere than at the top, say in a last section that might not remain the last. It was sanctioned by existing usage, however, and always under a heading.

As for breaking it up into some number between 2 and 11 PRs ... maybe, but it would be slightly more work to review, and we are only talking about colouring. :)

@willingc
Copy link
Collaborator

@willingc willingc commented Oct 23, 2017

@jeff5 Thanks for the detailed explanation. As a point of clarification for you, I was talking about comparing rendered versions built locally side by side.

I don't favour choosing the highlight language per block generally (strategy 2), as it makes the :: shorthand impossible and adds 3 lines each time.

I'm looking at maintenance over time. With strategy 2, one does still use :: and it adds 1 line of text with 2 blank lines. I don't feel strongly about strategy 1 versus 2, but, in practice, I do think that strategy 1 will be more error prone when it comes to individual code blocks.

I'll defer to @Mariatta, @ezio-melotti, @terryjreedy, and @merwok on selecting which strategy. I'm merely raising it so that we take pause and consider the longer term maintenance implications of approaches.

Thanks and as I said earlier I think being consistent with highlighting whatever the approach is a good thing 👍

@ezio-melotti ezio-melotti merged commit 6360d7f into python:master Oct 24, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jeff5
Copy link
Contributor Author

@jeff5 jeff5 commented Oct 24, 2017

Thanks all. 🍺

@jeff5 jeff5 deleted the jeff5:cp-highlight branch Oct 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.