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

Add missing code examples #45361

Merged
merged 2 commits into from
Oct 25, 2017
Merged

Add missing code examples #45361

merged 2 commits into from
Oct 25, 2017

Conversation

GuillaumeGomez
Copy link
Member

r? @rust-lang/docs

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2017
@QuietMisdreavus
Copy link
Member

Every doctest failed with something like this:

[01:20:32] ---- os/linux/fs.rs - os::linux::fs::MetadataExt::st_size (line 179) stdout ----
[01:20:32] 	error: unused import: `std::os::unix::prelude::*;`
[01:20:32]  --> os/linux/fs.rs:5:5
[01:20:32]   |
[01:20:32] 5 | use std::os::unix::prelude::*;
[01:20:32]   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
[01:20:32]   |
[01:20:32] note: lint level defined here
[01:20:32]  --> os/linux/fs.rs:1:9
[01:20:32]   |
[01:20:32] 1 | #![deny(warnings)]
[01:20:32]   |         ^^^^^^^^
[01:20:32]   = note: #[deny(unused_imports)] implied by #[deny(warnings)]
[01:20:32] 
[01:20:32] error[E0599]: no method named `st_size` found for type `std::fs::Metadata` in the current scope
[01:20:32]   --> os/linux/fs.rs:10:21
[01:20:32]    |
[01:20:32] 10 | println!("{}", meta.st_size());
[01:20:32]    |                     ^^^^^^^
[01:20:32]    |
[01:20:32]    = help: items from traits can only be used if the trait is in scope
[01:20:32]    = note: the following trait is implemented but not in scope, perhaps add a `use` for it:
[01:20:32]            candidate #1: `use std::os::linux::fs::MetadataExt;`

From the Cargo invocation that's printed at the end, it looks like it's properly running under the x86_64-unknown-linux-gnu target, and the linux module is tagged with #[doc(cfg(target_os = "linux"))] from that PR. It seems like the MetadataExt trait isn't in that prelude, somehow? This seems off.

@QuietMisdreavus
Copy link
Member

Oh! You're trying to use the unix::MetadataExt trait, but all these methods are on the linux::MetadataExt trait.

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2017
@GuillaumeGomez
Copy link
Member Author

Ah maybe.

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 18, 2017
@GuillaumeGomez
Copy link
Member Author

Let's see with this cfg. :)

@kennytm
Copy link
Member

kennytm commented Oct 18, 2017

o_O Why not just add use std::os::linux::fs::MetadataExt. #[doc(cfg)] should have ensured the doc tests won't run outside of Linux. The issue here is you did not import std::os::linux::fs::MetadataExt.

@GuillaumeGomez
Copy link
Member Author

It's working fine in a code of mine so I don't think this is the issue.

@GuillaumeGomez
Copy link
Member Author

My fix worked!

@QuietMisdreavus
Copy link
Member

You just #if 0'd all your code samples. There is no #[cfg(linux)]. The std::os module itself uses #[cfg(target_os = "linux")] to target Linux-specific things.

@kennytm
Copy link
Member

kennytm commented Oct 19, 2017

This should work, you could adapt it to the examples...

use std::fs;
use std::os::linux::fs::MetadataExt; // <--------------- use this -------------

use std::io;
fn f() -> io::Result<()> {
    let meta = fs::metadata("/dev/null")?;
    println!("{}", meta.st_dev());
    Ok(())
}

fn main() {
    println!("{:?}", f());
}

@GuillaumeGomez
Copy link
Member Author

Updated and working.

/// # Examples
///
/// ```
/// # #[cfg(target_os = "linux")] {
Copy link
Member

Choose a reason for hiding this comment

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

This #[cfg(target_os = "linux")] block could be removed, #[doc(cfg)] should not run the doc tests outside of Linux.

Copy link
Member Author

Choose a reason for hiding this comment

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

The point is that, if users want to take a look at the full code (by clicking on "run"), they'll see this conditional compilation.

Copy link
Member

Choose a reason for hiding this comment

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

How many people click on "Run" though? I'm thinking of how many people copy out the examples in File and run into "? in main" errors.

///
/// # Examples
///
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

no_run?

Copy link
Member

Choose a reason for hiding this comment

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

I guess since all of the examples are secretly function definitions that are never called, it's no difference whether or not no_run is actually there.

/// # use std::io;
/// # fn f() -> io::Result<()> {
/// let meta = fs::metadata("some_file")?;
/// let stat = meta.as_raw_stat();
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth adding an example to this method, since it's deprecated?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's always worth it. As long as it's there, it has to be documented.

Copy link
Member

Choose a reason for hiding this comment

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

Fair point. I was thinking in terms of "we're suggesting people use other methods, so why do we provide incentive to look at this one?"

Also now that i'm thinking about it, i'm concerned that this may not actually pass tests, since every doctest in std has deny(warnings) applied to it. If this passes travis, i'll let it slide, but that's at least my reasoning.

@QuietMisdreavus
Copy link
Member

Welp, travis passed and i have no idea how. If it's a problem then homu will find it; if it's not a problem then i'll stop worrying about it.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 24, 2017

📌 Commit e42da90 has been approved by QuietMisdreavus

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 25, 2017
…reavus

Add missing code examples

r? @rust-lang/docs
@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 25, 2017
bors added a commit that referenced this pull request Oct 25, 2017
Rollup of 5 pull requests

- Successful merges: #45361, #45461, #45465, #45486, #45502
- Failed merges:
@bors bors merged commit e42da90 into rust-lang:master Oct 25, 2017
@GuillaumeGomez GuillaumeGomez deleted the fs-docs branch October 25, 2017 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants