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

Convert all code listings to use new <Listing> preprocessor #3919

Closed
21 tasks done
chriskrycho opened this issue May 14, 2024 · 32 comments
Closed
21 tasks done

Convert all code listings to use new <Listing> preprocessor #3919

chriskrycho opened this issue May 14, 2024 · 32 comments

Comments

@chriskrycho
Copy link
Contributor

chriskrycho commented May 14, 2024

Background

As of #3918, we have a preprocessor that allows us to author with a custom HTML tag, Listing, roughly as if it were a component in a templating language. This input:

<Listing number="1-1" file-name="src/lib.rs" caption="A listing showing how to use a `Listing`">

```rust
fn main() {
    println!("Hello, listing!");
}
```

</Listing>

…will generate this output in the regular book (and strip all the tags in the NoStarch book):

<figure class="listing">
<span class="file-name">Filename: src/lib.rs</span>
<pre><pre class="playground"><code class="language-rust">fn main() {
    println!("Hello, listing!");
}</code></pre></pre>
<figcaption>Listing 1-2: A listing showing how to use a <code>Listing</code></figcaption>
</figure>

As described in the PR adding support for this, the result is more accessible HTML, which will also give us a nice way to hook in for styling things better if we so choose.

Contributing

When converting to a <Listing>, you can drop the leading Listing <number>: from the caption arg, since it handles that automatically with the number arg.

Do add a <Listing> for all code blocks (including rust, text, console, etc.) which have either or both of:

  • an explicit listing number, in the form <span class="caption">Listing XX-YY: …</span>
  • an explicit file name, in the form <span class="filename">Filename: src/main.rs</span>

Do not add a <Listing> for code blocks which do not have at least one or the other of those.

If you’d like to help, please leave a comment below noting which chapter you’d like to pick up so folks don’t do duplicate work! If it already has a user handle by it, please don’t work on that chapter.

@davidkurilla
Copy link
Contributor

davidkurilla commented May 16, 2024

I'd like to help with chapter 1.

@davidkurilla
Copy link
Contributor

Thanks for the feedback for my pervious contribution! I plan to work on converting chapters 2 and 3

@chriskrycho
Copy link
Contributor Author

Very good! Thank you! 💙

@SpectralPixel
Copy link
Contributor

I'll do chapters 6-10. Already working on it.

@bzierk
Copy link
Contributor

bzierk commented Jun 12, 2024

I am working on converting chapter 11.

@bzierk
Copy link
Contributor

bzierk commented Jun 12, 2024

I'm in the process of converting 12-15. It is mostly working but all captions which have angle brackets (eg Box<T>) are breaking the xml parser. Will work on handling the necessary escape characters.

@chriskrycho
Copy link
Contributor Author

Interesting – I would not have expected that! 🤔 Let me know what you come up with.

@bzierk
Copy link
Contributor

bzierk commented Jun 14, 2024

The issue appears to be that < is not a valid character in the body of an attribute in XML. These characters have to be escaped but with the ListingBuilder taking things by reference its a little trickier.

https://github.com/RazrFalcon/xmlparser/blob/e54c2768f20814140c11e6c92f94ee74bfd54808/src/lib.rs#L1037

@SpectralPixel
Copy link
Contributor

I've been quite busy over here, I hope to get back to work soon and finish the chapters.

@SpectralPixel
Copy link
Contributor

How do I build the mdbook with the Listing pre-processor? If I recall correctly, <Listing> support hasn't been upstreamed yet, so how do I test the book? Or is that responsibility offloaded to code reviewers? Thanks :)

@SpectralPixel
Copy link
Contributor

SpectralPixel commented Jun 26, 2024

Also, what about code that is preceded by a filename, yet doesn't have a listing number? Are these also turned into <Listing>s that don't have a number or caption?
(Edit: I'll do these as well for now, feel free to revert them later.)

@chriskrycho
Copy link
Contributor Author

@SpectralPixel sorry, just saw these questions.

  1. For the listing preprocessor, it worked just fine in this repo anyway, and it has been fixed in the rust repo, so nothing to worry about there.
  2. Mmmm, those are interesting – we probably need to think about how <Listing> should handle those. I will poke at how many of those there are and figure out what we ought to do with them!

@SpectralPixel
Copy link
Contributor

Hmmm, I already converted those special <Listing>s, so I'll add them in my PR. Feel free to revert those commits if necessary though. I'll try to finish it all up this afternoon.

@SpectralPixel
Copy link
Contributor

Weird, mdbook seems not to work on my machine when I install it the way that is recommended for this repo... I guess I'll just send the PR so you can check if it works fine, it's probably just me doing something wrong with mdbook...

@chriskrycho
Copy link
Contributor Author

@SpectralPixel if you consistently have issues with mdbook not working, would you open an issue? That way we can track fixing that—it’s important to us to make sure folks can contribute easily!

@SpectralPixel
Copy link
Contributor

Well, I'm not sure if I have it set up properly...

@chriskrycho
Copy link
Contributor Author

@SpectralPixel @bzierk I believe #3975 should address the issues each of you were separately hitting in terms of what the <Listing> preprocessor supports!

@jpmelos
Copy link
Contributor

jpmelos commented Sep 26, 2024

I did chapter 4 here: #4043.

I think some explanation about which listings need to be converted would be helpful:

  • Just the ones with listing number, caption, or file name? Or also the ones without any of those things?
  • Only Rust listings, or all of them (even the console ones, for example)?

@chriskrycho
Copy link
Contributor Author

chriskrycho commented Sep 27, 2024

Thanks for the note, @jpmelos – I’ll update the issue description here. Basically, it should only be converted if it has an explicit listing number (<span class="caption">Listing XX-YY: …</span>), an explicit file name (<span class="filename">Filename: …</span>), or both. Otherwise, it isn’t a “listing” in the way we approach it, so should not be converted!

@jpmelos
Copy link
Contributor

jpmelos commented Oct 1, 2024

I did chapter 05 here: #4051.

@chriskrycho
Copy link
Contributor Author

@SpectralPixel would you like me to fix up those PRs you opened and get them across the line? (No worries; I have very often been the guy who started something and then life got busy!)

@SpectralPixel
Copy link
Contributor

Oh, I must've forgotten to revert those few commits. Should be only one per PR, otherwise they're all ready to go.

@LifeAdventurer
Copy link
Contributor

I'd like to help with chapter 16.

@chriskrycho
Copy link
Contributor Author

@LifeAdventurer go for it – I’ll mark you down on the list! Thanks!

@chriskrycho
Copy link
Contributor Author

@SpectralPixel see the PRs—they’re all currently failing; I think it’s primarily a function of line-wrapping (see my comment on #3977 for more).

@SpectralPixel
Copy link
Contributor

I did all the line wrapping in at most one commit per PR, could you revert these? I'm quite busy at the moment. Other than that, the PRs should be ready to go. :)

@chriskrycho
Copy link
Contributor Author

Ah, got it—that’ll work!

@LifeAdventurer
Copy link
Contributor

@LifeAdventurer go for it – I’ll mark you down on the list! Thanks!

Thanks! I'll get started on it now.

@LifeAdventurer
Copy link
Contributor

@chriskrycho While working on this, I noticed the changes in your PR #4060 for a different chapter and saw that a <span> tag might have been unintentionally left in the listing:

<Listing number="18-2" file-name="src/lib.rs" caption="Listing 18-2: Implementations of the public methods `add`, `remove`, and `average` on `AveragedCollection`">
<span class="filename">Filename: src/lib.rs</span>
```rust,noplayground
{{#rustdoc_include ../listings/ch18-oop/listing-18-02/src/lib.rs:here}}
```
</Listing>

Also, both the text Listing 18-1: and Listing 18-2: still remain in the caption:

<Listing number="18-1" file-name="src/lib.rs" caption="Listing 18-1: An `AveragedCollection` struct that maintains a list of integers and the average of the items in the collection">

<Listing number="18-2" file-name="src/lib.rs" caption="Listing 18-2: Implementations of the public methods `add`, `remove`, and `average` on `AveragedCollection`">

Could you check if they need to be removed? If needed, I can open another pull request to fix it.

@chriskrycho
Copy link
Contributor Author

Thanks for noting that, @LifeAdventurer – I went ahead and fixed it this morning, and am reviewing your PR for ch. 16 now!

@LifeAdventurer
Copy link
Contributor

Thanks for noting that, @LifeAdventurer – I went ahead and fixed it this morning, and am reviewing your PR for ch. 16 now!

Glad I could help with that. Thanks for merging the PR! Let me know if there's anything else I can help with.

@chriskrycho
Copy link
Contributor Author

All right, as of #4072, which re-applied @bzierk’s changes to Chapter 15 now that they’re unblocked, we’re done here. Thanks for your help, everyone! 🎉

@chriskrycho chriskrycho unpinned this issue Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants