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

Reapply: Move hiding of boring lines into static content (#846) #1065

Merged
merged 2 commits into from
Oct 16, 2019

Conversation

benediktwerner
Copy link
Contributor

Was reverted because of formatting in #1064

@Dylan-DPC-zz Dylan-DPC-zz merged commit 84a2ab0 into rust-lang:master Oct 16, 2019
@ehuss
Copy link
Contributor

ehuss commented Nov 1, 2019

@benediktwerner This appears to have broken hidden line removal in some cases. Do you think you can take a look for a fix?

Here are some examples:
https://doc.rust-lang.org/nightly/reference/patterns.html#range-patterns
https://doc.rust-lang.org/nightly/reference/runtime.html#the-panic_handler-attribute

@benediktwerner
Copy link
Contributor Author

@ehuss Sure, although I didn't create the original PR I just took a quick look at it and created #1088 which I think fixes the issue in the first link.

The issue in the second link is that the code block is marked with ignore and is therefore not converted to a playpen: https://github.com/benediktwerner/mdBook/blob/master/src/renderer/html_handlebars/hbs_renderer.rs#L597

I'm not sure what the best fix for this would be. Is it even correct that code blocks like that aren't converted to a playpen? According to the documentation ignore only prevents rustdoc from running the code.

@ehuss
Copy link
Contributor

ehuss commented Nov 4, 2019

I think it is correct that ignored blocks should not use the playpen, since they are unlikely to work.

Would it work to process all # lines for all rust code blocks to match the old behavior?

@benediktwerner
Copy link
Contributor Author

The thing is ignored blocks don't even have a button to show hidden lines, so the only way for these lines to show up is when using the copy button. Also, the example in the link runs just fine on rust playground and otherwise there is also the no_run tag that seems to only hide the run button.

So I think the example in the link should be displayed in a playpen and if some code is not runnable it should use ignore,no_run but still be displayed in a playpen with the "Show hidden lines" button.

@benediktwerner
Copy link
Contributor Author

Also, the noplaypen class should probably be removed as well. The no_run class is enough to hide the run button but doesn't prevent hiding lines. For compatibility, it could be changed to do the same as no_run. Although, if there is another reason for noplaypen to exist then that would be a good reason to hide lines even without a playpen but as far as I can tell from the PR it was added only to hide the play button.

@ehuss
Copy link
Contributor

ehuss commented Nov 6, 2019

I see what you're saying that there is not a strong reason for ignore blocks to have hidden lines. However, it might be useful in some cases, such as temporarily disabling an example, or using the hidden lines to convey intent to people working on the documentation, but not important to readers. I agree the runtime example shouldn't be ignore, but it needed to be when it was written. Now we can remove the ignore field, and it will work without needing to rewrite it.

I think noplaypen was intentionally added to have something to be run by mdbook test, but is otherwise not desired for the user to try to run. no_run would disable the testing the code.

@benediktwerner
Copy link
Contributor Author

I actually do think there might be reasons to have hidden lines in ignore blocks. What I want to say is that ignore blocks should simply become playpens because the ignore should only target rustdoc and people should use no_run or noplaypen if they don't want a run button or a playpen. If ignore blocks become playpens lines would be hidden properly without any other change and there would also be a button to show the hidden lines. So basically I want to remove the check for ignore here: https://github.com/benediktwerner/mdBook/blob/master/src/renderer/html_handlebars/hbs_renderer.rs#L612

In any case, I now changed my PR to always hide lines as long as the language is rust so it doesn't matter that much anymore.

Ruin0x11 pushed a commit to Ruin0x11/mdBook that referenced this pull request Aug 30, 2020
…) (rust-lang#1065)

* Move hiding of boring lines into static content (rust-lang#846)

* Fix test for hidden code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants