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 lots of docs. Rust doc days! #11

Merged
merged 2 commits into from Jun 27, 2016

Conversation

Projects
None yet
5 participants
@brson
Contributor

brson commented Jun 24, 2016

No description provided.

src/lib.rs Outdated
/// it goes out of scope.
///
/// The [`TempDir`] type creates a directory on the file system that
/// is deleted once it goes out of scope. At construction the

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jun 24, 2016

"At construction the TempDir creates a new directory with a randomly generated name, and with a prefix of your choosing."

I have a small issue with "at construction". Seems like a comma is needed or something?

This comment has been minimized.

@brson

brson Jun 24, 2016

Contributor

Yes.

src/lib.rs Outdated
/// to ensure that no further file system operations are attempted
/// inside the temporary directory once it has been deleted.
///
/// Various platformm-specific conditions may cause `TempDir` to fail

This comment has been minimized.

@GuillaumeGomez
src/lib.rs Outdated
/// # Examples
///
/// ```
/// extern crate tempdir;

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jun 24, 2016

In here, you can remove extern crate tempdir; and the main function as well.

src/lib.rs Outdated
/// # Examples
///
/// ```
/// extern crate tempdir;

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jun 24, 2016

The same goes for all code examples.

src/lib.rs Outdated
/// fn main() {
/// let tmp_dir = TempDir::new("example").expect("create temp dir");
///
/// // Check that the temp directory actually exists

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jun 24, 2016

Missing a dot at the end of the sentence.

src/lib.rs Outdated
/// assert!(tmp_dir.path().exists());
/// }
/// ```
///

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jun 24, 2016

Extra empty line.

src/lib.rs Outdated
/// Unwrap the wrapped `std::path::Path` from the `TempDir` wrapper.
/// This discards the wrapper so that the automatic deletion of the
/// temporary directory is prevented.
/// Unwrap the [`Path`] contained in the `TempDir`, and

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jun 24, 2016

I think the comma is too much.

src/lib.rs Outdated
/// # Examples
///
/// ```
/// extern crate tempdir;

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jun 24, 2016

Remove extra useless code here as well.

src/lib.rs Outdated
/// fs::remove_dir_all(tmp_path).expect("remove temp dir");
/// }
/// ```
///

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jun 24, 2016

Extra empty line.

src/lib.rs Outdated
/// # Examples
///
/// ```
/// extern crate tempdir;

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jun 24, 2016

Extra unneeded code.

@brson brson force-pushed the brson:next branch from 39e5cb7 to 8f505aa Jun 24, 2016

@brson

This comment has been minimized.

Contributor

brson commented Jun 24, 2016

Addressed all points. Thanks for review!

src/lib.rs Outdated
/// Unwrap the wrapped `std::path::Path` from the `TempDir` wrapper.
/// This discards the wrapper so that the automatic deletion of the
/// temporary directory is prevented.
/// Unwrap the [`Path`] contained in the `TempDir` and

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jun 24, 2016

Here you write "unwrap", but just above you wrote "Attempts". Any reason that it doesn't use third singular pronum here?

src/lib.rs Outdated
}

/// Close and remove the temporary directory
/// Close and remove the temporary directory, returing a `Result`

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jun 24, 2016

Same question here.

@brson brson force-pushed the brson:next branch from 8f505aa to 8e90b65 Jun 24, 2016

@brson

This comment has been minimized.

Contributor

brson commented Jun 24, 2016

I switched them all to third person. Good catch!

@GuillaumeGomez

This comment has been minimized.

GuillaumeGomez commented Jun 24, 2016

All good for me. Great work @brson!

src/lib.rs Outdated
///
/// After creating a `TempDir`, work with the file system by doing
/// standard [`std::fs`] file system operations on it's [`Path`],
/// which can be retrieved with [`TempDir::path`]. Once the `TempDir`

This comment has been minimized.

@jbcden

jbcden Jun 25, 2016

Could we maybe say TempDir.path? I see TempDir::path and instantly think associated functions and not ones that operate on references. I am not sure if this is a standard thing to do or not just a thought.

This comment has been minimized.

@killercup

killercup Jun 26, 2016

I think it's fine to say TempDir::path here, as it is the UFCS and refers to a method. That specific method just happens to take &self as first parameter :)

This comment has been minimized.

@brson

brson Jun 27, 2016

Contributor

I'll leave as is.

src/lib.rs Outdated
/// everything inside it will be automatically deleted once the
/// returned `TempDir` is destroyed.
///
/// If the directory can not be created, `Err` is returned.

This comment has been minimized.

@jbcden

jbcden Jun 25, 2016

For consistency perhaps we should add an Errors header here.

This comment has been minimized.

@jbcden

jbcden Jun 25, 2016

s/can not/cannot

This comment has been minimized.

@brson

brson Jun 27, 2016

Contributor

Yes.

src/lib.rs Outdated
/// location by constructing with [`TempDir::new_in`].
///
/// After creating a `TempDir`, work with the file system by doing
/// standard [`std::fs`] file system operations on it's [`Path`],

This comment has been minimized.

@jbcden
src/lib.rs Outdated
//! The [`TempDir`] type creates a directory on the file system that
//! is deleted once it goes out of scope. At construction, the
//! `TempDir` creates a new directory with a randomly generated name,
//! and with a prefix of your choosing.

This comment has been minimized.

@jbcden

jbcden Jun 25, 2016

I think this might flow better without the "with". Thoughts?

This comment has been minimized.

@brson

brson Jun 27, 2016

Contributor

Yes.

src/lib.rs Outdated
//!
//! The [`TempDir`] type creates a directory on the file system that
//! is deleted once it goes out of scope. At construction, the
//! `TempDir` creates a new directory with a randomly generated name,

This comment has been minimized.

@jbcden

jbcden Jun 25, 2016

I don't believe the "," is necessary here.

src/lib.rs Outdated
//!
//! fn main() {
//! // Create a directory inside of `std::env::temp_dir()`, named with
//! // the prefix, "example".

This comment has been minimized.

@jbcden

jbcden Jun 25, 2016

Not sure if the "," before "example" is needed.

src/lib.rs Outdated
//! let mut tmp_file = File::create(file_path).expect("create temp file");
//! writeln!(tmp_file, "Brian was here. Briefly.").expect("write temp file");
//!
//! // By closing the `TempDir` explicitly we can check that it has

This comment has been minimized.

@jbcden

jbcden Jun 25, 2016

A comma after "explicitly" might be nice.

src/lib.rs Outdated
/// and with a prefix of your choosing.
///
/// The default constructor, [`TempDir::new`], creates directories in
/// the location returned by [`std::env::temp_dir()`], but they can be

This comment has been minimized.

@jbcden

jbcden Jun 25, 2016

I am uncertain what "they" is referring to here.

This comment has been minimized.

@killercup

killercup Jun 26, 2016

I guess: s/they/TempDir

And further down: "by constructing it with"

src/lib.rs Outdated
///
/// The default constructor, [`TempDir::new`], creates directories in
/// the location returned by [`std::env::temp_dir()`], but they can be
/// configured to manage the lifetime of a temporary directory in any

This comment has been minimized.

@jbcden

jbcden Jun 25, 2016

Perhaps "lifetime" is not the best choice of word here given its significance in Rust.

This comment has been minimized.

@brson

brson Jun 27, 2016

Contributor

I actually purposefully connected them, since the lifetime of the variable in Rust is tied to the lifetime of the directory on disk. But maybe 'scope' would be better. Agree it could be confusing so Ill reword.

@jbcden

This comment has been minimized.

jbcden commented Jun 25, 2016

Not a problem with your changes but when I render the documentation I am seeing:

fn into_path(self) -> PathBuf

but into_path takes mut self. Any idea why that wouldn't show?

src/lib.rs Outdated
/// let file_path = tmp_dir.path().join("my-temporary-note.txt");
/// let mut tmp_file = File::create(file_path).expect("create temp file");
/// writeln!(tmp_file, "Brian was here. Briefly.").expect("write temp file");
/// ```

This comment has been minimized.

@killercup

killercup Jun 26, 2016

Maybe add comment after the last line:

// `tmp_dir` goes out of scope, the directory as well as `tmp_file` will be deleted here.

This comment has been minimized.

@brson

brson Jun 27, 2016

Contributor

Ok.

src/lib.rs Outdated
/// everything inside it will be automatically deleted once the
/// returned `TempDir` is destroyed.
///
/// If the directory can not be created, `Err` is returned.

This comment has been minimized.

@killercup

killercup Jun 26, 2016

dito for the "Errors" headline. Maybe mention io::Result? (Kind of redundant though as it's already in the signature)

src/lib.rs Outdated
///
/// // Check that the temp directory actually exists.
/// assert!(tmp_dir.path().exists());
/// ```

This comment has been minimized.

@killercup

killercup Jun 26, 2016

Idea for another example:

use tempdir::TempDir;

let tmp_path;

{
    let tmp_dir = TempDir::new("example").expect("create temp dir");
    let tmp_path = tmp_dir.path().clone();

    // Check that the temp directory actually exists.
    assert!(tmp_path.exists());

    // End of `tmp_dir` scope, directory will be deleted
}

// Temp directory should be deleted by now
assert_eq!(tmp_path.exists(), false);

This comment has been minimized.

@brson

brson Jun 27, 2016

Contributor

Thanks, I'll add it.

This comment has been minimized.

@brson

brson Jun 27, 2016

Contributor

Actually I'll just use yours instead since it's additive to mine.

///
/// Although `TempDir` removes the directory on drop, in the destructor
/// any errors are ignored. To detect errors cleaning up the temporary
/// directory, call `close` instead.
///

This comment has been minimized.

@killercup

killercup Jun 26, 2016

Here, an "Errors" section might be especially helpful even if just for mentioning usual errors and linking to io::Result.

This comment has been minimized.

@brson

brson Jun 27, 2016

Contributor

Good idea.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Jun 27, 2016

Nice! r=me with comments addressed

@GuillaumeGomez

This comment has been minimized.

GuillaumeGomez commented Jun 27, 2016

Not a problem with your changes but when I render the documentation I am seeing:

fn into_path(self) -> PathBuf

but into_path takes mut self. Any idea why that wouldn't show?

I think this is a rustdoc problem. Can you open an issue on rustc and attribute it to me please?

@jbcden

This comment has been minimized.

jbcden commented Jun 27, 2016

@brson

This comment has been minimized.

Contributor

brson commented Jun 27, 2016

but into_path takes mut self. Any idea why that wouldn't show?

Probably because the mutability of the variable isn't part of the interface; it doesn't matter to callers.

@brson

This comment has been minimized.

Contributor

brson commented Jun 27, 2016

Updated.

@alexcrichton alexcrichton merged commit 1b77f7d into rust-lang-deprecated:master Jun 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment