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

Use fs::symlink_metadata in doc for is_symlink #39176

Merged
merged 4 commits into from
Jan 22, 2017

Conversation

CartesianDaemon
Copy link
Contributor

fs::metadata() follows symlinks so is_symlink() will always return
false. Use symlink_metadata instead in the example in the
documentation.

See issue #39088.

fs::metadata() follows symlinks so is_symlink() will always return
false. Use symlink_metadata instead in the example in the
documentation.

See issue rust-lang#39088.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@ranma42
Copy link
Contributor

ranma42 commented Jan 19, 2017

Somewhat related, the docs for Metadata mentions that it is returned from the std::fs::metadata function. It might be a good idea to show that std::fs::symlink_metadata is another way to get a Metadata value.

EDIT: fix link

@@ -975,13 +975,18 @@ impl FileType {

/// Test whether this file type represents a symbolic link.
///
/// The Metadata struct needs to be retreived with
Copy link
Member

Choose a reason for hiding this comment

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

This place wants more inline-code annotations, so it looks like

retrieved with fs::symlink_metadata() not fs::metadata().

and not

retrieved with fs::symlink_metadata() not fs::metadata().

You can achieve that enclosing the inline code snippets within the grave (`) symbols. Like this:

retrieved with `fs::symlink_metadata()` not `fs::metadata()`.

@@ -975,13 +975,18 @@ impl FileType {

/// Test whether this file type represents a symbolic link.
///
/// The Metadata struct needs to be retreived with
Copy link
Member

Choose a reason for hiding this comment

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

"retreived" -> "retrieved" maybe?

@@ -975,13 +975,18 @@ impl FileType {

/// Test whether this file type represents a symbolic link.
///
/// The Metadata struct needs to be retreived with
/// fs::symlink_metadata() not fs::metadata(). metadata()
Copy link
Member

Choose a reason for hiding this comment

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

Missing urls.

@alexcrichton
Copy link
Member

@bors: r+

Thanks @CartesianDaemon!

@bors
Copy link
Contributor

bors commented Jan 20, 2017

📌 Commit fe9f5d5 has been approved by alexcrichton

@CartesianDaemon
Copy link
Contributor Author

Thank you for your help!

@frewsxcv
Copy link
Member

@bors rollup

@bors
Copy link
Contributor

bors commented Jan 21, 2017

⌛ Testing commit fe9f5d5 with merge 09a7eae...

@bors
Copy link
Contributor

bors commented Jan 21, 2017

💔 Test failed - status-appveyor

@frewsxcv
Copy link
Member

frewsxcv commented Jan 21, 2017

@bors retry

@bors
Copy link
Contributor

bors commented Jan 21, 2017

☔ The latest upstream changes (presumably #39203) made this pull request unmergeable. Please resolve the merge conflicts.

@frewsxcv
Copy link
Member

@CartesianDaemon Any reason for that last commit where you got rid of the link?

@CartesianDaemon
Copy link
Contributor Author

It looked to me like the duplicate patch (#39203) got merged before this, so it's a conflict and I need to not make that change (or rebase off the more recent master). Is there a better thing to do?

@frewsxcv
Copy link
Member

Ah, that make sense. Thanks for your contribution!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 21, 2017

📌 Commit f55bbaa has been approved by frewsxcv

@CartesianDaemon
Copy link
Contributor Author

Thanks!

@bors
Copy link
Contributor

bors commented Jan 22, 2017

⌛ Testing commit f55bbaa with merge 1b06375...

bors added a commit that referenced this pull request Jan 22, 2017
Use fs::symlink_metadata in doc for is_symlink

fs::metadata() follows symlinks so is_symlink() will always return
false. Use symlink_metadata instead in the example in the
documentation.

See issue #39088.
@bors
Copy link
Contributor

bors commented Jan 22, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: frewsxcv
Pushing 1b06375 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants