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

Implement playpen support for ```rust #222

Merged
merged 3 commits into from Mar 10, 2017
Merged

Implement playpen support for ```rust #222

merged 3 commits into from Mar 10, 2017

Conversation

steveklabnik
Copy link
Member

Fixes #29

So, this all works, and fixes an important regression in mdbook for the rust book.

However.

The big issue remaining is the "wrap in main" rules from rustdoc. In other words, something like this:

```rust
let x = 5;
```

will fail, as it needs to be inside of main.

I'm not sure what the right way is to go here, exactly. I'm imagining duplicating the logic of rustdoc here makes sense.

Thoughts @azerupi @carols10cents @frewsxcv ?

let text = &caps[1];
let classes = &caps[2];

if classes.contains("language-rust") && !classes.contains("ignore") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about should_panic? i can't decide whether not having a run button or having a run button that then causes a panic would be more confusing, just want to make sure we consider it :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a good question.

I'm not sure of the answer.

@steveklabnik
Copy link
Member Author

Here's the logic from rustdoc:

pub fn maketest(s: &str, cratename: Option<&str>, dont_insert_main: bool,
                opts: &TestOptions) -> String {
    let (crate_attrs, everything_else) = partition_source(s);

    let mut prog = String::new();

    // First push any outer attributes from the example, assuming they
    // are intended to be crate attributes.
    prog.push_str(&crate_attrs);

    // Next, any attributes for other aspects such as lints.
    for attr in &opts.attrs {
        prog.push_str(&format!("#![{}]\n", attr));
    }

    // Don't inject `extern crate std` because it's already injected by the
    // compiler.
    if !s.contains("extern crate") && !opts.no_crate_inject && cratename != Some("std
") {
        if let Some(cratename) = cratename {
            if s.contains(cratename) {
                prog.push_str(&format!("extern crate {};\n", cratename));
            }
        }
    }
    if dont_insert_main || s.contains("fn main") {
        prog.push_str(&everything_else);
    } else {
        prog.push_str("fn main() {\n");
        prog.push_str(&everything_else);
        prog = prog.trim().into();
        prog.push_str("\n}");
    }

    info!("final test program: {}", prog);

    prog

so, doing this too shouldn't be too hard.

@steveklabnik
Copy link
Member Author

steveklabnik commented Mar 6, 2017

I pushed up a commit to inject mains. The attribute stuff is a little harder, but not necessary.

One issue...

untitled

Because the older style was linking to the playpen, these warnings would just happen, and it did nothing about it. so this isn't strictly speaking a regression.

There's still a lot of polish to do here but I think this gets us 99% of the way

@steveklabnik
Copy link
Member Author

Discussing ⬆️ with @carols10cents , we think auto-injecting allow(unused_variables) is probably the best route. I'll add another commit soonish.

@steveklabnik
Copy link
Member Author

Done. @azerupi how are you feeling about this PR?

@azerupi
Copy link
Contributor

azerupi commented Mar 10, 2017

It's good 👍

Obviously, we can't continue to add special cases like this forever. But until we tackle the bigger architectural changes, it's fine to handle it this way :)

@steveklabnik
Copy link
Member Author

Obviously, we can't continue to add special cases like this forever. But until we tackle the bigger architectural changes, it's fine to handle it this way :)

Awesome, I feel the same way. My intention is to try to look at the bigger stuff after we get all of this little stuff fixed as we go 😄

@steveklabnik steveklabnik merged commit c6a5d12 into rust-lang:master Mar 10, 2017
@steveklabnik
Copy link
Member Author

Oh, and do you think we could get a release? We have two or three things now; it'd be good to get this out in front of people

@steveklabnik steveklabnik deleted the gh29 branch March 10, 2017 16:59
@azerupi
Copy link
Contributor

azerupi commented Mar 10, 2017

Oh, and do you think we could get a release?

Well, it's a perfect occasion to test if you can publish to crates.io too ;)
I added you to the maintainers, so it should work. Let me know how it goes.

@steveklabnik
Copy link
Member Author

0.0.18 published! ❤️

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.

Support embedding of rust playpen examples which are runnable
5 participants