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

Set playground-url value to default to "play.rust-lang.org" #86363

Closed
wants to merge 2 commits into from

Conversation

GuillaumeGomez
Copy link
Member

Now, by default, the "Run" button will appear on all rust code blocks.

r? @jyn514

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 16, 2021
.opt_str("playground-url")
.map(|s| s.trim().replace("\"", "").replace("\'", ""))
.or_else(|| Some("https://play.rust-lang.org/".to_owned()))
.filter(|s| !s.is_empty());
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 allows to pass an empty value if we want to disable it.

@@ -15,5 +15,5 @@
/// ```
pub fn foo() {}

// @!has hidden_line/fn.foo.html invisible
// @!has hidden_line/fn.foo.html '// invisible'
Copy link
Member Author

Choose a reason for hiding this comment

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

Since it now appears in the url, I needed to make the check a bit bigger.

// @!has - 'space'
// @!has - 'comment'
// @!has - ':space'
// @!has - ':comment'
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above.

@@ -1,3 +1,5 @@
// compile-flags: --playground-url="" -Z unstable-options
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 test now ensures that we can disable it by providing an empty value to playground-url option.

@ehuss
Copy link
Contributor

ehuss commented Jun 16, 2021

I'm a little unclear on this change, does this enable the playground for all projects, even though --playground-url is unstable?

Does it not work in most situations, since the playground won't have the source for most projects? Also, does the playground have the capacity for potentially a lot more traffic? AIUI, the playground has been occasionally timing out lately with the load that it already has. This also seems like it could be a hazard for closed-source projects which may not want to accidentally share code externally (even just examples)?

@GuillaumeGomez
Copy link
Member Author

I'm a little unclear on this change, does this enable the playground for all projects, even though --playground-url is unstable?

The attribute is stable:

#![doc(html_playground_url = "some.url")]

Also, it can be disabled by setting its value to an empty string. The reason behind this is that I often found myself wanting to edit the whole code (including hidden lines) but couldn't.

Does it not work in most situations, since the playground won't have the source for most projects?

I don't see an issue with that.

Also, does the playground have the capacity for potentially a lot more traffic? AIUI, the playground has been occasionally timing out lately with the load that it already has.

Even though it's a fair concern, I'm not sure it's really relevant here...

This also seems like it could be a hazard for closed-source projects which may not want to accidentally share code externally (even just examples)?

Like said above, it can be disabled. However, I'm not sure how high the risk would be though... It's simply an example after all, they're supposed to show how to use their API, not how the crate source code looks like.

@jyn514
Copy link
Member

jyn514 commented Jun 21, 2021

Even though it's a fair concern, I'm not sure it's really relevant here...

It definitely is, rustdoc should not be sending a ton of traffic to playground if it can't handle it. That just makes the experience worse for everyone. You should at least coordinate with @shepmaster to see if there's anything simple playground can do to make this easier.

@jyn514 jyn514 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 21, 2021
@GuillaumeGomez
Copy link
Member Author

to see if there's anything simple playground can do to make this easier.

What do you mean? Having a website with just the code for editing and not running?

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. and removed T-rustdoc-temp labels Jun 22, 2021
@shepmaster
Copy link
Member

Does it not work in most situations, since the playground won't have the source for most projects?

I don't see an issue with that.

It does seem non-ideal to link people to a website dedicated to running Rust code when we expect that the vast majority of time they wouldn't be able to run it. It'd be kind of like linking to docs.rs when we know ahead of time that a library hasn't been documented.

Also, does the playground have the capacity for potentially a lot more traffic? AIUI, the playground has been occasionally timing out lately with the load that it already has.

The timeouts have been associated with running code; loading the editor and some source code should not be affected. If people purely want to look at the code, they shouldn't experience any issues.

If they click run (or build or ...) then they will be increasing the load.

It is my hope that we identify the load problems on the playground and address them, but there hasn't been much time spent on that, so I can't really give any guarantees one way or the other.


Overall, I'm not extremely worried from a performance perspective. The user experience of non-compilable code seems more concerning.

@GuillaumeGomez
Copy link
Member Author

At some point we generated the run buttons all the time and I never heard anyone complained about that. However, maybe some other people did hear about it and also, our user base is much bigger today as well. I'm not sure how to check this though... If anyone has a suggestion?

@jyn514 jyn514 added I-needs-decision Issue: In need of a decision. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jul 2, 2021
@jyn514
Copy link
Member

jyn514 commented Jul 2, 2021

@rust-lang/rustdoc do you have options on this? I'm mildly inclined to close but I don't feel strongly. In particular I'm looking for an opinion on this:

Does it not work in most situations, since the playground won't have the source for most projects?

I don't see an issue with that.

It does seem non-ideal to link people to a website dedicated to running Rust code when we expect that the vast majority of time they wouldn't be able to run it. It'd be kind of like linking to docs.rs when we know ahead of time that a library hasn't been documented.

@Nemo157
Copy link
Member

Nemo157 commented Jul 2, 2021

I agree that a "Run" button linking to non-compiling code is not a good look. If the usecase is to expand #-hidden lines in the example, that seems like a separate feature that could be done entirely inline instead of linking to another site.

@GuillaumeGomez
Copy link
Member Author

This is why I asked if we had a code editor website. But then we should replace "Run" with another sentence.

@Nemo157
Copy link
Member

Nemo157 commented Jul 2, 2021

By inline I mean entirely inline in the HTML that rustdoc generates, the user can then copy the entire example out elsewhere. It may not even have to offer a way to view the entire example, maybe just a "copy with hidden code" button and then the user can do whatever they want with it.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Jul 2, 2021

Or simply a "copy" button. I really like the idea! (which would always copy all the code)

@Manishearth
Copy link
Member

I've been surprised this feature doesn't already exist multiple times and have wanted to make this change myself.

I don't really see a problem here, I don't expect this to cause a huge load on the playground, and if it does we can potentially get support from the foundation. We shouldn't be making product decisions based on that; we should make the decision we think is good for rustdoc, and if load on the playground turns out to be an issue, we can talk to the infra team and foundation to get support for implementing our product decision.

Does it not work in most situations, since the playground won't have the source for most projects?

This isn't how this works, the relevant source is passed in as a URL param. The playground doesn't need to have access to any of the source beforehand.


I actually don't think a normal expand button would be better UX than the run button that we currently have. The run button is pretty nice, though it might be worth sprucing up its UI (a floating play triangle seems better than a button)

@ehuss
Copy link
Contributor

ehuss commented Jul 2, 2021

This isn't how this works, the relevant source is passed in as a URL param.

I think you may have misunderstood what I meant. Any use statement for a crate or module not in the top 100 will fail to build. That should essentially be almost every example except for the top 100 crates.

@Manishearth
Copy link
Member

I think you may have misunderstood what I meant. Any use statement for a crate or module not in the top 100 will fail to build. That should essentially be almost every example except for the top 100 crates.

oh! I see.

Yeah, that's a good point, and reason enough to not do this I think.

@jyn514
Copy link
Member

jyn514 commented Jul 3, 2021

I'm going to close this since it sounds like it won't work in most cases. I think the "Copy" button idea (#86363 (comment)) is great, I'd love to see a PR for that :)

@jyn514 jyn514 closed this Jul 3, 2021
@GuillaumeGomez GuillaumeGomez deleted the plyaground branch July 3, 2021 22:05
@GuillaumeGomez
Copy link
Member Author

Going to open an issue then!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-needs-decision Issue: In need of a decision. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants