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

Roles/styles for good/bad code blocks in PEPs #22

Closed
encukou opened this issue Feb 14, 2022 · 18 comments · Fixed by python/peps#3574
Closed

Roles/styles for good/bad code blocks in PEPs #22

encukou opened this issue Feb 14, 2022 · 18 comments · Fixed by python/peps#3574
Labels
discussion doc:peps Related to PEPs peps.python.org

Comments

@encukou
Copy link
Member

encukou commented Feb 14, 2022

Do we want to add styling for good/bad code blocks to PEPs?

For an epxample, look at pep8.org, a third-party (and outdated) rendering of PEP 8
The main difference between it and the new style proposed for PEPs is the red & green indicators on what to do vs. what not to do.

I've just read the dangerous "naive" SQL code in PEP-0675 and thought it could use a similar "don't copy this" indicator. There surely are many other examples.
Of course everything should be covered in prose for screen readers and color-blind people, but for many people colors could aid understanding.

Would it be useful in docs as well?

@AA-Turner
Copy link
Member

AA-Turner commented Feb 15, 2022

This would be reasonably easy with

.. code:: python
   :class: bad

& similar for good -- it is then just a stylesheet change.

We could add .. code-good:: and .. code-bad:: directives, but I think the first example is better as it requires no change to the parsing, so will be picked up by e.g. GitHub's RST renderer.

A

@encukou
Copy link
Member Author

encukou commented Feb 16, 2022

I don't know much about where it should be discussed -- perhaps Discourse would be better.
But IMO it would be good to bring it up after/if PEP 676 (PEP Infrastructure Process) lands, so the change is actually visible, and docs about it can be added to PEP 12 (Sample reStructuredText PEP Template).

@CAM-Gerlach
Copy link
Member

Sounds like a great change to me, and @AA-Turner 's approach makes sense—so long as there is a way to clearly communicate to doc writers how and when they should use it (or is this just for PEPs?)

Also, @AA-Turner , any reason to prefer .. code:: vs. .. code-block::, especially if this is deferred until after PEP 676?

@pradyunsg
Copy link
Member

pradyunsg commented Feb 16, 2022

They're aliases, so there isn't any functional difference between the two.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Feb 16, 2022

They're aliases, so there isn't any functional difference between the two.

Hmm, well then if that's the case, the Sphinx and the Docutils documentation should be updated, then—Docutils only lists .. code (though .. code-block, at least without options, appears to work just fine in practice on the legacy PEP builder, just without syntax highlighting—perhaps Docutils has added support or @AA-Turner added it as a custom directive), which only has three allowed options (number-lines, plus name and class), while Sphinx's docs only list .. code-block with only sourcecode as an alias, which has a plethora of options but does not include number-lines, which is instead named linenos; nor class, which is instead recommended to be added with Sphinx's .. rst-class:: directive instead—and thus, if the docs are correct, is the answer to my question as to why @AA-Turner used .. code:: instead, for compatibility with the legacy docutils builder.

If we instead defer this to after if/when PEP 676 is adopted, as @encukou suggests and we seem to agree on, we can use the modern Sphinx semantics with .. code-block:: and .. rst-class instead of the legacy .. code:: and :class:.

@pradyunsg
Copy link
Member

pradyunsg commented Feb 16, 2022

Oh, wait, I'm wrong! And, you are absolutely correct.

code is defined in docutils.

sourcecode and code-block are aliases, both defined in Sphinx. This has more properties and configurable items than code.

@pradyunsg
Copy link
Member

I personally think using :class: on .. code-block is a good idea, but let's cross that bridge when we get to it. Let's wait on PEP 676 to get decided on. :)

@AA-Turner
Copy link
Member

code-block works with pure Docutils, check the language aliases.

Will update when on PC.

A

@pradyunsg
Copy link
Member

pradyunsg commented Feb 16, 2022

Indeed! There's an en.py that has:

      'code': 'code',
      'code-block': 'code',
      'sourcecode': 'code',

I should've looked closer. I guess my 20-ish year old brain's memory isn't completely trash yet. :)

Sphinx also has a code directive -- https://github.com/sphinx-doc/sphinx/blob/8b23f6db12d9fcdb6e1ca2015ea32e3e2e1fbdb6/sphinx/directives/patches.py#L147

@pradyunsg
Copy link
Member

So... final update:

  • code, code-block and sourcecode work with both Sphinx and docutils.
  • They're all aliases, in docutils, for English content.
  • code is different-ish from code-block/sourcecode in Sphinx -- you can emphasis line numbers and do a couple more things with code-block but not code.

@CAM-Gerlach
Copy link
Member

I personally think using :class: on .. code-block is a good idea, but let's cross that bridge when we get to it. Let's wait on PEP 676 to get decided on. :)

I'm assuming you mean .. rst-class with .. code-block? The .. code-block:: directive doesn't have the legacy :class: option like .. code::, as that is now handled with the modern .. rst-class directive with Sphinx (confirmed via testing).

But otherwise, 👍 to the above.

@pradyunsg
Copy link
Member

The .. code-block:: directive doesn't have the legacy :class: option like .. code::,

It does have that tho. https://github.com/sphinx-doc/sphinx/blob/a55a765e797197cfdf8a3e5033efe8ed72191a62/sphinx/directives/code.py#L114

Also, I'm not really sure why you consider passing an option to a directive to be "legacy" -- could you elaborate on why you think that's "legacy"?

@CAM-Gerlach
Copy link
Member

It does have that tho. https://github.com/sphinx-doc/sphinx/blob/a55a765e797197cfdf8a3e5033efe8ed72191a62/sphinx/directives/code.py#L114

Hmm, well upon further testing it does appear to work (I somehow missed seeing the class on the outer div), but it appears to be undocumented (despite :name: being fully documented), likely for backward-compat with docutils, nor is it documented on any other Sphinx directives or elsewhere. Also, on both docutils and Sphinx, aligning the :class: option with the beginning of the directive but then using standard four-space indents for the block content injects an extra pre-pended space into the rendered output.

Also, I'm not really sure why you consider passing an option to a directive to be "legacy" -- could you elaborate on why you think that's "legacy"?

Its not that passing an option to a directive in the general case is "legacy", but rather this specific case of a bespoke :class: option to this specific directive seems to be, given it is undocumented while elsewhere the documented, standard method of applying a class to any reST element is the rst-class directive, which also avoids the aforementioned rendering issue when using standard 4-space code indents but 3-space directive option spacing.

@AA-Turner
Copy link
Member

bespoke :class: option

generally with reST you can pass :class: to any directive (entirely from memory, untested, and still on mobile so won't search for documentation). Note also the evolution of code and code-block is odd, with Sphinx implementing code-block first and Docutils later adding code.

A

@willingc willingc added the doc:peps Related to PEPs peps.python.org label May 3, 2022
@willingc willingc changed the title Roles/styles for good/bad code blocks Roles/styles for good/bad code blocks in PEPs May 3, 2022
@encukou
Copy link
Member Author

encukou commented Dec 4, 2023

FWIW, a devguide page now uses emoji directly:

@hugovk
Copy link
Member

hugovk commented Dec 5, 2023

PEP 676 has landed, please see PR python/peps#3567 to add green/red borders to the left of good/bad code examples of PEP 8.

We can add them to other PEPs afterwards.

@hugovk
Copy link
Member

hugovk commented Dec 5, 2023

For the examples of good/bad commits in the devguide: python/devguide#1237

@hugovk
Copy link
Member

hugovk commented Dec 9, 2023

python/devguide#1237 has been merged to add the CSS classes and apply them to PEP 8.

I've just read the dangerous "naive" SQL code in PEP-0675 and thought it could use a similar "don't copy this" indicator. There surely are many other examples.

Please see python/peps#3574 to add a red sidebar here.

We can close this issue now the CSS is available, we can add to other PEPs as and when. 🚀

@hugovk hugovk closed this as completed Dec 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion doc:peps Related to PEPs peps.python.org
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants