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

Conditionally show compile warnings when running code #1315

Closed
wants to merge 5 commits into from

Conversation

xiaochuanyu
Copy link

@xiaochuanyu xiaochuanyu commented Sep 6, 2020

This PR makes it so that warnings are shown if a markdown code block is labelled with warn e.g.

```rust,warn
fn main() {
    let unused = 7;
}
```

This enables addressing issues like rust-lang/rust-by-example#1328.

Summary of changes:

  • Updated to use execute API to run code (https://play.rust-lang.org/)
  • Adjust javascript to create additional result block to display stderr which has the warnings conditionally

Here is a demo:
example

Copy link
Contributor

@azerupi azerupi left a comment

Choose a reason for hiding this comment

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

I wanted to ask if it would show all compiler output (always) or only in case of error, but I saw in the code that there is a match on the word warning on the output. The keyword matches the behaviour.

I think it is good.

@ehuss
Copy link
Contributor

ehuss commented Sep 22, 2020

Thanks for the PR! I have a few questions and concerns.

  • Why change the API endpoint? What are the differences? cc @shepmaster, this changes the playground API. I'm not familiar with the differences, can you comment on what you would prefer?
  • This seems like it could be confusing with examples that don't have fn main, which have an implicit #![allow(unsued)].
  • What is the expected scenarios that this would be added? Why would it not be a global setting?
  • Why does it have separate code elements for the two outputs? It seems like the majority of examples don't have any output, and this results in two code blocks, where one is usually going to be empty. Could they be combined?
  • This renames one of the documentation chapters, which should probably include a redirect for the old page.

@shepmaster
Copy link
Member

can you comment on what you would prefer?

Thank you for asking1 ❤️!

At this point in time, the only public API that the playground guarantees will never change is /evaluate.json, which has existed effectively forever in Rust terms. Breaking this would break exciting things like a huge swath of stable documentation.

All other endpoints are internal implementation details. To my recollection, no one has ever suggested working to create a stable API for the playground.


1 — This may be the first time that anyone has ever actually asked about using an endpoint before doing so!

@shepmaster
Copy link
Member

@Dylan-DPC-zz
Copy link

Dylan-DPC-zz commented Sep 22, 2020

1 — This may be the first time that anyone has ever actually asked about using an endpoint before doing so!

:D

we are prone to bikeshedding everything :P

@xiaochuanyu
Copy link
Author

xiaochuanyu commented Sep 29, 2020

Responding to @ehuss above:

Why change the API endpoint? What are the differences? cc @shepmaster, this changes the playground API. I'm not familiar with the differences, can you comment on what you would prefer?

I chose execute because evaluate did not provide warnings. On compilation success, it would return stdout which excluded warnings (from stderr). On compilation failure, it would return stdout + stderr.
On the other hand, execute provided both stderr and stdout all the time among other fields.
Given the comments from @shepmaster above it sounds like execute is internal and not supposed to be used. I'm not aware of any other options :(

This seems like it could be confusing with examples that don't have fn main, which have an implicit #![allow(unsued)].

Good point, I haven't thought about this case. I may look into if we can adjust the logic so that #![allow(unsued)] is not set if warnings are opted in.

What is the expected scenarios that this would be added? Why would it not be a global setting?

The use case I was thinking about was the various examples in Rust By Example where we expect users to play around with given code and thus having warnings would be a benefit to learning the language.
A global option might work as well I think. I wasn't that sure exactly how to go about this so I simply implemented most flexible option I thought of at the time. I'm open to suggestions here.

Why does it have separate code elements for the two outputs? It seems like the majority of examples don't have any output, and this results in two code blocks, where one is usually going to be empty. Could they be combined?

No strong reason here except it looked better visually to me to have two separate blocks of error/warning (stderr) vs. stdout.
Also note that I added a code block specifically for stderr but its only shown if there are warnings and starts off hidden. You should not observe two blocks if there are no warnings or error (or at least thats my intention).

            var show_warnings = should_warn && response.stderr.includes("warning");
            // show stderr block if there is compile error or warning
            if (!response.success || show_warnings) {
                result_stderr_block.innerText = response.stderr;
                result_stderr_block.classList.remove("hidden");
            }

This renames one of the documentation chapters, which should probably include a redirect for the old page.

Is there an example of how to do this?

@xiaochuanyu
Copy link
Author

Haven't had time to look into this and it doesn't look like there is much interest anyway.
Closing.

@mgeisler
Copy link
Contributor

Haven't had time to look into this and it doesn't look like there is much interest anyway.

I'm interested in this feature 😄 I wrote https://github.com/google/comprehensive-rust and there we quickly noticed the lack of stderr reporting back for our examples. The biggest problem is that it means that we cannot use dbg! in code snippets.

Alx-Lai added a commit to Alx-Lai/comprehensive-rust that referenced this pull request Oct 6, 2024
Add a stderr block.
Pros:
  - we can make use of dbg macro in our code
Cons:
  - there's a limitation that the compile message also shows

Applies patches from rust-lang/mdBook#1315 since the original change
was not adopted by rust-lang.

Signed-off-by: Alx-Lai <alexabc722@gmail.com>
Alx-Lai added a commit to Alx-Lai/comprehensive-rust that referenced this pull request Oct 6, 2024
Add a stderr block.
Pros:
  - we can make use of dbg macro in our code
Cons:
  - there's a limitation that the compile message also shows

Applies patches from rust-lang/mdBook#1315 since the original change
was not merged by rust-lang.

Signed-off-by: Alx-Lai <alexabc722@gmail.com>
Alx-Lai added a commit to Alx-Lai/comprehensive-rust that referenced this pull request Oct 7, 2024
Add a stderr block.
Pros:
  - we can make use of dbg macro in our code
Cons:
  - there's a limitation that the compile message also shows

To be improved:
  - compile message regex may change overtime
  - can use websocket to replace the current approach

Applies patches from rust-lang/mdBook#1315 since the original change
was not merged by rust-lang.

Signed-off-by: Alx-Lai <alexabc722@gmail.com>
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.

6 participants