Skip to content

/walkdir

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

Use `?` in docs instead of unwrapping #52

Merged
merged 1 commit into from Jun 30, 2017

Conversation

Reviewers

@budziq budziq

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@msehnout @budziq @BurntSushi
@msehnout
Copy link
Contributor

msehnout commented Jun 16, 2017

Fixes: #25 except for "examples on WalkDirIterator.skip_current_dir" because there is no unwrap()
@budziq

budziq suggested changes Jun 17, 2017
View changes

Copy link
Contributor

budziq left a comment

Hi @msehnout

Nice work! I would suggest few minor changes.
.travis.yml Outdated
@@ -1,6 +1,6 @@
language: rust
rust:
- 1.10.0
- 1.18.0

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Jun 17, 2017 •
edited

Contributor

I guess that the whole point of this travis entry was to check for backwards compatibility with older rust versions (not check with the latest stable 😄 as it is done in travis entry below)

Please note that "?" became stable in 1.13.0. @BurntSushi will have to decide if he wants to drop backwards compatibility with 1.10 for the sake of "?" in examples. Otherwise the try! macro would have to be used instead.

This comment has been minimized.

Sign in to view
Copy link
@msehnout

msehnout Jun 17, 2017

Author Contributor

Sorry, I didn't know :-) Anyway I was following this guideline:
https://github.com/brson/rust-api-guidelines#examples-use--not-try-not-unwrap-c-question-mark
src/lib.rs Outdated
@@ -67,7 +79,8 @@ This uses the `filter_entry` iterator adapter to avoid yielding hidden files
and directories efficiently:
```rust,no_run
use walkdir::{DirEntry, WalkDir, WalkDirIterator};

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Jun 17, 2017

Contributor

why add newline?

This comment has been minimized.

Sign in to view
Copy link
@msehnout

msehnout Jun 17, 2017

Author Contributor

Sorry, typo
src/lib.rs Outdated
@@ -28,12 +28,18 @@ The following code recursively iterates over the directory given and prints
the path for each entry:
```rust,no_run
use walkdir::WalkDir;
use walkdir::{Error, WalkDir};

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Jun 17, 2017

Contributor

i would suggest splitting the use here into two lines and biding the Error line as it should be part of the hidden error boilerplate.

this concerns all examples below.

This comment has been minimized.

Sign in to view
Copy link
@msehnout

msehnout Jun 17, 2017

Author Contributor

Sure, no problem.
src/lib.rs Outdated
# fn try_main() -> Result<(), Error> {

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Jun 17, 2017

Contributor

as far as i know the convention is fn run() but its not super important.

This comment has been minimized.

Sign in to view
Copy link
@msehnout

msehnout Jun 17, 2017

Author Contributor

Again, I was following this guideline and they use try_main() there:
https://github.com/brson/rust-api-guidelines#examples-use--not-try-not-unwrap-c-question-mark

This comment has been minimized.

Sign in to view
Copy link
@budziq

budziq Jun 17, 2017

Contributor

I stand corrected 😄

@msehnout msehnout force-pushed the msehnout:master branch 2 times, most recently from b984671 to b2e3670 Jun 19, 2017

@msehnout

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

msehnout commented Jun 19, 2017

@budziq I've submitted a new version of this PR with fixed issues and reverted travis.yml.
@budziq

This comment has been minimized.

Sign in to view
Copy link
Contributor

budziq commented Jun 19, 2017

@msehnout reverting the travis.yml might be a little bit to early as now the travis build for backwards compatibility (rust 1.10.0) fails.

Please note that the ? operator was stabilized later in rust 1.13 so any builds with ? will always fail unless:

  1. @BurntSushi decides to move minimal backwards compatible version to 1.13 and you update the travis.yml
  2. you change ? to try!

I would suggest option 1. but it is not my place to decide 😸

I would suggest updating the PR to make it pass travis test. Chose one of the two options and wait for maintainers approval or suggestions.
@msehnout

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

msehnout commented Jun 22, 2017

@budziq I think I'll just wait for @BurntSushi's opinion.

@KodrAus KodrAus referenced this pull request Jun 26, 2017

Closed

Update Minimal Rust Version Support #61

@BurntSushi

This comment has been minimized.

Sign in to view
Copy link
Owner

BurntSushi commented Jun 26, 2017

Since we're releasing a 2.0, I think we should bump the minimum Rust version as well! I think we should move to Rust 1.16, which is $CURRENT - 2.

@KodrAus KodrAus referenced this pull request Jun 26, 2017

Merged

Derive Debug for public structs #59

@budziq budziq referenced this pull request Jun 27, 2017

Merged

Bumped the minimal required rust version to 1.16.0 #62

@msehnout

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

msehnout commented Jun 29, 2017

Sounds good to me. Once the version in travis is changed, I can rebase this patch to run CI again.
@msehnout

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

msehnout commented Jun 29, 2017

Or should I bump it?
@budziq

This comment has been minimized.

Sign in to view
Copy link
Contributor

budziq commented Jun 29, 2017

@msehnout version bum is already waiting for merge #62
@BurntSushi

This comment has been minimized.

Sign in to view
Copy link
Owner

BurntSushi commented Jun 29, 2017

Thanks! I think this is ready to go but needs a rebase first.
@msehnout
Use `?` in docs instead of unwrapping 
Fixes: #25
Loading status checks…
672499f
@msehnout

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

msehnout commented Jun 29, 2017

Ok, I have the fix, but I cannot authenticate against Github. Seems to be broken using either https or ssh. I'll submit the patch later.

@msehnout msehnout force-pushed the msehnout:master branch from b2e3670 to 672499f Jun 30, 2017

@msehnout

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

msehnout commented Jun 30, 2017

It should be ready.

@BurntSushi BurntSushi merged commit 938b0fa into BurntSushi:master Jun 30, 2017
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@BurntSushi

This comment has been minimized.

Sign in to view
Copy link
Owner

BurntSushi commented Jun 30, 2017

Awesome, thank you!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.