Eliminate horizontal scrolling in rust code blocks about comprehensive-rust HOT 14 OPEN

We already had the code for redbox, We should just need to add the trigger event when playground code lines crossed.

It sounds like you're talking about JavaScript code? I'm talking about something running in a GitHub Action (a script written in Python/Rust/...).

I was talking about javascript code.

from comprehensive-rust.

Hi @1dimir, thanks for bringing this up!

Due to line numbering, there is less space left for the code. My observations are that the scrolling threshold is 83 for less than 10 line long code, 82 - less than 100 and 81 - above 100. There are no snippets above 1k and I suppose there must not be in the context of the course.

Oh, good point! I've probably forgotten to update the width after we turned on the line numbers.

You're completely right that we should not have code blocks with 1k lines in this course. The longest code blocks I know of are in 56.1. RTC Driver and related exercises. I think they're mostly there because @qwandor developed a tool which will generate zip files with the code blocks. I would prefer the code blocks to be removed from the pages (#625).

I've wondered if we could implement something which will issue warnings or errors if it sees more than N lines of code on a single slide. @djmitche has been working on splitting up large slides together with @mani-chand (#1464).

There should be an automated format validation step. rustfmt can do so with rust files, but not markdown.

Luckily, dprint fmt will format the Rust code in the Markdown files! This was done in #1157 and it was the main reason for implementing google/mdbook-i18n-helpers#19.

Please see the Formatting section for all the tools to install 😄 Let us know if this works/doesn't work for you!

from comprehensive-rust.

I've wondered if we could implement something which will issue warnings or errors if it sees more than N lines of code on a single slide.

Hello @mgeisler , Could explain it bit in detail , You mean the code should not cross a specific number of lines.

Can you specify how many lines you would like to have a playground. I would suggest if it crossed that specific number of lines we will display the redbox (#1842) as warning.

We can use editor api to get number of lines , If it excides then we will display redbox as warning.

from comprehensive-rust.

I've wondered if we could implement something which will issue warnings or errors if it sees more than N lines of code on a single slide.

Hello @mgeisler , Could explain it bit in detail , You mean the code should not cross a specific number of lines.

Can you specify how many lines you would like to have a playground. I would suggest if it crossed that specific number of lines we will display the redbox (#1842) as warning.

Ah, I had not thought of that. It could be interesting for local development.

However, the warning/error I'm talking about here has to be generated when people make the PR — it should not show up on the deployed course on https://google.github.io/comprehensive-rust/ since that page is used by people who want to learn Rust. Warnings would be very disruptive there 😄

from comprehensive-rust.

Yes, I am speaking the same it's should be in local development while merging PR. We have made redbox for local development if you remembered.

When a PR is created regarding the change in slide.Reviewer can easily understand that it crosses number of lines as redbox popup when the slide is visited in local development.

from comprehensive-rust.

I've wondered if we could implement something which will issue warnings or errors if it sees more than N lines of code on a single slide.

Hello @mgeisler , Could explain it bit in detail , You mean the code should not cross a specific number of lines.

Can you specify how many lines you would like to have a playground. I would suggest if it crossed that specific number of lines we will display the redbox (#1842) as warning.

Ah, I had not thought of that. It could be interesting for local development.

However, the warning/error I'm talking about here has to be generated when people make the PR — it should not show up on the deployed course on https://google.github.io/comprehensive-rust/ since that page is used by people who want to learn Rust. Warnings would be very disruptive there 😄

We already had the code for redbox, We should just need to add the trigger event when playground code lines crossed.

If you are okay then we can add it in #1942 PR.

from comprehensive-rust.

We already had the code for redbox, We should just need to add the trigger event when playground code lines crossed.

It sounds like you're talking about JavaScript code? I'm talking about something running in a GitHub Action (a script written in Python/Rust/...).

from comprehensive-rust.

We already had the code for redbox, We should just need to add the trigger event when playground code lines crossed.

It sounds like you're talking about JavaScript code? I'm talking about something running in a GitHub Action (a script written in Python/Rust/...).

Ok .now, I am not about writing a script in github actions. I will check if i can do it and let you know.

from comprehensive-rust.

@mgeisler, hi!

Due to line numbering, there is less space left for the code. My observations are that the scrolling threshold is 83 for less than 10 line long code, 82 - less than 100 and 81 - above 100. There are no snippets above 1k and I suppose there must not be in the context of the course.

Oh, good point! I've probably forgotten to update the width after we turned on the line numbers.

An update on thresholds. There are few places, where a code block is placed inside Speaker Notes. That area has additional margins, that further decrease available space for the editable code:

86 characters seems to fit in readonly blocks regardless of their placement.

from comprehensive-rust.

Luckily, dprint fmt will format the Rust code in the Markdown files! This was done in #1157 and it was the main reason for implementing google/mdbook-i18n-helpers#19.

Please see the Formatting section for all the tools to install 😄 Let us know if this works/doesn't work for you!

Thanks! I missed that and used only rustfmt following CI steps. Although it doesn't work exactly as I expected.

Prerequisites: rustfmt.toml: max_width = 85, dprint.json: "lineWidth": 80.

dprint check indeed works on both .md and .rs files. For rust files it uses minimal width value among the configurations. For texts inside .md it applies lineWidth threshold. But it doesn't look like it affects anyhow contents of code blocks inside .md.

from comprehensive-rust.

I started thinking of browsing the book with playwright/selenium/etc to check for actual scrolling occurrences...

from comprehensive-rust.

Thanks! I missed that and used only rustfmt following CI steps.

I think our setup instructions are a little confusing (#1426), so please let me know if I've messed up and written about rustfmt somewhere?

For rust files it uses minimal width value among the configurations. For texts inside .md it applies lineWidth threshold. But it doesn't look like it affects anyhow contents of code blocks inside .md.

Oh! I must admit that I haven't looked deeply into this myself, I was just happy to see it reformat the code blocks 🙂

I did some testing just now and I think you might have seen the effects of caching. If I run

dprint fmt --incremental=false

then I do see the Rust code change after an update to rustfmt.toml. The line widths seem independent: one for Markdown and one for Rust.

from comprehensive-rust.

It sounds like you're talking about JavaScript code? I'm talking about something running in a GitHub Action (a script written in Python/Rust/...).

I was talking about javascript code.

Okay, thanks for confirming — I suggest you don't do anything here since we'll need a different tool than JavaScript here.

from comprehensive-rust.

I started thinking of browsing the book with playwright/selenium/etc to check for actual scrolling occurrences...

That would be very interesting! I've been waiting for someone to show up with the necessary skills for something like that 😄 I would be interested in statistics that show how tall and wide each slide is.

If we have tooling for this, then I could imagine having a warning on a PR that makes the page taller than a given threshold — perhaps even with a screenshot like I did manually in #1464.

An update on thresholds.

Thank you for conducting this test! I tried reformatting the code in 80 columns and I think it is okay. A few of the changes can be counter-acted by simply trimming the code. As an example, iterator/intoiterator.md has an example with x_coords and y_coords, but I think calling them xs and ys is equally fine. That fits nicely in 80 columns.

from comprehensive-rust.