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 module examples and general docs edits. #53

Merged
merged 1 commit into from Jun 27, 2016

Conversation

Projects
None yet
4 participants
@terrynsun
Contributor

terrynsun commented Jun 26, 2016

No description provided.

src/lib.rs Outdated
//! for entry in glob("/media/**/*.jpg").unwrap() {
//! if let Ok(path) = entry {
//! println!("{:?}", path.display())
//! }

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jun 26, 2016

Maybe add an example in case it returns Err?

src/lib.rs Outdated
//!
//! for entry in glob("/media/**/*.jpg").unwrap() {
//! if let Ok(path) = entry {
//! println!("{:?}", path.display())

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jun 26, 2016

Just wondering: isn't a semicolon (';') missing here?

This comment has been minimized.

@terrynsun

terrynsun Jun 26, 2016

Contributor

Technically no, I think it just uses the value of println to be the value of the if-let block. But this was a typo on my part anyway.

src/lib.rs Outdated
//! ```rust
//! use glob::glob;
//!
//! for entry in glob("/media/**/*.jpg").unwrap() {

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jun 26, 2016

Are you sure this code works? The glob function returns a Result type which you unwrap, so you cannot use pattern binding on entry, right?

This comment has been minimized.

@killercup

killercup Jun 26, 2016

This should work, @GuillaumeGomez . glob() returns a Result<Paths, PatternError>, and Path::iter() yields Result<PathBuf, GlobError> items, so you need unwrap/match both.

This comment has been minimized.

@killercup

killercup Jun 26, 2016

I would change the .unwrap() to .expect("Failed to read glob pattern"), though.

src/lib.rs Outdated
//! require_literal_separator: false,
//! require_literal_leading_dot: false,
//! };
//! for entry in glob_with("local/*a*", &options).unwrap() {

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jun 26, 2016

Same comment as above.

src/lib.rs Outdated
@@ -223,8 +253,8 @@ pub fn glob_with(pattern: &str, options: &MatchOptions) -> Result<Paths, Pattern
/// A glob iteration error.
///
/// This is typically returned when a particular path cannot be read
/// to determine if its contents match the glob pattern. This is possible
/// if the program lacks the permissions, for example.
/// to determine if its contents match the glob pattern. For example,

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jun 26, 2016

I preferred the old sentence.

@GuillaumeGomez

This comment has been minimized.

GuillaumeGomez commented Jun 26, 2016

This is globally good, nice work! Take a look at my comments.

src/lib.rs Outdated
//!
//! To print all jpg files in `/media/` and all of its subdirectories:
//!
//! ```rust

This comment has been minimized.

@killercup

killercup Jun 26, 2016

I guess there are no files in /media/ on Travis CI where the doc tests will run, so you might want to make this rust,no_run (cf. the book for more information). Otherwise, it probably won't hurt since you just iterate through zero entries.

src/lib.rs Outdated
@@ -398,9 +428,9 @@ impl fmt::Display for PatternError {

/// A compiled Unix shell style pattern.
///
/// `?` matches any single character
/// `?` matches any single character.

This comment has been minimized.

@killercup

killercup Jun 26, 2016

I'd probably change this list of pattern syntax items to be an actual markdown list :)

@killercup

This comment has been minimized.

killercup commented Jun 26, 2016

Looks good to me, aside from a few nits.

Bonus points if you add #![deny(missing_docs)]! 😄

@terrynsun

This comment has been minimized.

Contributor

terrynsun commented Jun 26, 2016

Thanks for the comments! I think I addressed all of them. 😄

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Jun 27, 2016

Thanks!

@alexcrichton alexcrichton merged commit 7714144 into rust-lang-nursery: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