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

stop auto-wrapping code snippets in fn main() #955

Closed
wants to merge 1 commit into from

Conversation

mikemorris
Copy link

I'm guessing this would be a semver breaking change, but would be happy to do the accompanying work of migrating code snippets in https://github.com/rust-lang/book to include fn main() {} wrapping explicitly (and #![allow(unused_variables)] where necessary) if this change is accepted (and the book migration could actually happen first because of the current text.contains("fn main") exclusion).

This seems to be a somewhat persistent issue in code snippet formatting in the Rust book, particularly for src/lib.rs files, with some awkward fixes (see expanded Listing 11-12 in https://doc.rust-lang.org/stable/book/ch11-03-test-organization.html for implications of this approach, extraneous code still added) without a particularly good solution (using the ignore class seems undesirable).

An alternative implementation could be adding support for a class like nomain or nowrap instead of changing this default behavior.

Refs #222 for initial implementation, #283 (quick_main! macro), #618, #712, rust-lang/book#1206, rust-lang/book#1874 (editable prevents fn main() wrapping), rust-lang/book#1982

Examples of problematic code snippets in the Rust book:

@carols10cents
Copy link
Member

Note that this is only changing the rendering for the playground snippet presented in the HTML. Rustdoc also does auto main wrapping, and mdbook test uses rustdoc's testing, so the awkward fixes in the book that allow them to be not ignored like in rust-lang/book#1339 would still need to happen unless rustdoc gains the ability to specify in another way that the code shouldn't be wrapped in main.

@Dylan-DPC-zz
Copy link

The next release is most likely going to be a breaking change so that shouldn't matter

@mikemorris
Copy link
Author

mikemorris commented Jun 16, 2019

@carols10cents Thanks for mentioning the wrapping in rustdoc, wasn't aware of that - I took a look into rustdoc and it looks like there's some pretty well-tested functionality around dont_insert_main and already_has_main in https://github.com/rust-lang/rust/blob/master/src/librustdoc/test.rs.

Should rustdoc add "contents contain #[cfg(test)] or #[test]" as an exclusion case for wrapping (is this technically allowed alongside a main function or not?) and/or would adding a rustdoc attribute for skipping the wrapping behavior be a possibility?

Does rustdoc actually run test blocks like in the code snippet in rust-lang/book#1339?

@carols10cents
Copy link
Member

You'd have to ask the rustdoc team those questions; I'd recommend opening an issue on rust-lang/rust and tagging rust-lang/rustdoc.

@ehuss
Copy link
Contributor

ehuss commented Jun 17, 2019

Yea, I don't think this can be done in this way. Whatever the solution is will need to start with rustdoc. Some options:

  • rustdoc could be smarter about what it wraps in main (seems very hard).
  • tell rustdoc that a code block is a "test" and it should build and run it with the harness (seems tricky).
  • add an annotation that something should compile (as a lib?), but not be run/tested.

There are probably better ideas out there.

Another question is if you expect the #[test] functions to actually run. Currently they are only checked that they compile.

@ehuss
Copy link
Contributor

ehuss commented Mar 9, 2020

I think the solution for this will likely need to start with rustdoc. When rustdoc has better main wrapping smarts, we can revisit updating mdbook.

@ehuss ehuss closed this Mar 9, 2020
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