[Docs]: Highlighting code snippets in .rst docs

[0xisk]

What did I change?

• Enhancing all the .rst doc files by adding highlighting for the code snippets, including the following langs:

1. solidity
2. assembly
3. bash
4. javascript

Thanks so much!

[cameel]

Thanks for the PR! Here's some feedback from me:

• Some of these snippets are mislabeled. For example some text shippets are marked as bash and some solidity ones are really yul.
• Should we use bash or shell for shell snippets?
• I'm not sure if we actually want to replace all the :: bits with code-block::. I personally prefer the code-block:: but :: is already pretty prevalent so we should first discuss this change.
• Please squash your commits. Small commits are nice but simply putting every file in a separate commit does not really help reviewing. One way to split it that would actually be helpful would be if different highlighting languages were in different commits. But a single commit would be fine too since all of these modifications are closely related.

Also, @axic mentioned at some point that he already had some stashed work cleaning this up so this might conflict with his changes.

[chriseth]

@cameel can you check that the script that extracts code snippets from the documentation still works with these changes? I assume it does, but please double-check.

[cameel]

Sure. I'll check it when I get back to finish #11420.

[0xisk]

Thanks for the PR! Here's some feedback from me:

Thanks so much for your feedback.

• Some of these snippets are mislabeled. For example some text shippets are marked as bash and some solidity ones are really yul.

Regarding Yul and Solididyt conflicting, it was just because of my lack of experience with Yul. I will go over the changes again to make sure of that.

• Should we use bash or shell for shell snippets?

For my I based my selection to the most used one, so before my changes .. code-block:: bash was used more in all the docs:

1. 4 times in docs/smtchecker.rst
2. 2 times in docs/layout-of-source-files.rst
3. 23 times in docs/installing-solidity.rst

While .. code-block:: shell was used just 1 time in docs/using-the-compiler.rst

Also, based on the following answer from StackOverflow on the Difference between sh and bash:

sh = bash?
For a long time, /bin/sh used to point to /bin/bash on most GNU/Linux systems. As a result, it had almost become safe to ignore the difference between the two. But that started to change recently.

So I think we could follow with .. code-block:: bash since it's the most used one already.

• I'm not sure if we actually want to replace all the :: bits with code-block::. I personally prefer the code-block:: but :: is already pretty prevalent so we should first discuss this change.

Based on the rst documentation in this section:

To show example code, use the reStructured Text code-block directive: Show Example Code

So in order to highlight an example snippet of code, we need to use code-block

• Please squash your commits. Small commits are nice but simply putting every file in a separate commit does not really help reviewing. One way to split it that would actually be helpful would be if different highlighting languages were in different commits. But a single commit would be fine too since all of these modifications are closely related.

Yes, I agree with you, I'm working on squashing commits now.

[0xisk]

• Please squash your commits. Small commits are nice but simply putting every file in a separate commit does not really help reviewing. One way to split it that would actually be helpful would be if different highlighting languages were in different commits. But a single commit would be fine too since all of these modifications are closely related.

Commits squashing is done.

[chriseth]

conf.py mentions highlight_language = 'Solidity' - can we somehow rely on this default so that we do not have to specify the language for all the solidity code examples?

[0xisk]

conf.py mentions highlight_language = 'Solidity' - can we somehow rely on this default so that we do not have to specify the language for all the solidity code examples?

The intention is to have the code highlighted in GitHub, to be more readable.

[cameel]

@iskanderandrews Note that the docs are primarily meant to be viewed after being rendered by Sphinx (e.g. on https://docs.soliditylang.org), for which we do have Yul highlighting via a pygments plugin. Good highlighting on github is a good thing if possible but if there's a conflict between the too, I'd rather choose good highlighting in Sphinx over github.

But I think it's a good argument for not relying on the default language in conf.py. If github highlights it as solidity only if we specify the language explicitly then I think it's better to do so.

[chriseth]

What do you mean by "highlighted in github"? Can you link to an example page on github?

[0xisk]

What do you mean by "highlighted in github"? Can you link to an example page on github?

Sure, so this is an example docs/common-patterns.rst from both repos:

• Main ethereum/solidity:
  https://github.com/ethereum/solidity/blob/develop/docs/common-patterns.rst

• Forked version iskanderandrews/solidity
  https://github.com/iskanderandrews/solidity/blob/feature/docs-highlight-solc/docs/common-patterns.rst

So that's what I mean by "highlighted in github" thus making it more readable.

[cameel]

@chriseth

@cameel can you check that the script that extracts code snippets from the documentation still works with these changes? I assume it does, but please double-check.

It does. In #11565 I now have tests that show that it works both with :: and .. code-block:: solidity. It does not really have any of them hard-coded. It finds all indented blocks in the docs and uses a heuristic to determine if they're Solidity code (i.e. checks for the SPDX comment, version pragma of a contract/library/interface declaration).

[cameel]

@iskanderandrews Sorry, for not getting back to this earlier. I finally had time to finish my related PR (#11420) so now I'm also back to reviewing this.

So I think we could follow with .. code-block:: bash since it's the most used one already.

Fair enough. Let's keep bash.

Apart from that there are 3 remaining issues:

1. Some blocks are still mislabeled (mostly solidity instead of yul but also solidity where it should be javascript and a few others). I pointed them all out in comments.
2. Some minor mistakes like .. cod::e-block:: in one place.
3. I still think asm blocks should be marked as text. I mean, either way it's not a big deal but the generic asm highlighting is not very good for them in my opinion. Maybe we need another opinion here.

Once that's done please rebase on develop and I think we can merge it :)

[cameel]

Is there actually a difference here? If it was a newline github would show that but it does not so I wonder what's going on here.

[0xisk]

Yeah it's weird I didn't catch out what's changed here. I think in this script I just highlighted Solidity.

[0xisk]

@iskanderandrews Sorry, for not getting back to this earlier. I finally had time to finish my related PR (#11420) so now I'm also back to reviewing this.

So I think we could follow with .. code-block:: bash since it's the most used one already.

Fair enough. Let's keep bash.

Apart from that there are 3 remaining issues:

1. Some blocks are still mislabeled (mostly solidity instead of yul but also solidity where it should be javascript and a few others). I pointed them all out in comments.
2. Some minor mistakes like .. cod::e-block:: in one place.
3. I still think asm blocks should be marked as text. I mean, either way it's not a big deal but the generic asm highlighting is not very good for them in my opinion. Maybe we need another opinion here.

Once that's done please rebase on develop and I think we can merge it :)

Alright, I think all has been resolved, I've rebased then squashed all commits. So, please review again, and mention me for any other changes or edits.
Thank you so much!

[cameel]

Thanks! It looks good now so I'm marking as approved.

I see that CI is not running on this PR, probably because it's an external one. @chriseth Do these need to be triggered manually now?

[chriseth]

It should run, not sure why it does not - or maybe it just does not report its status?