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

Path methods — symlinks improvement #85747

Merged
merged 6 commits into from
Jun 18, 2021

Conversation

maxwase
Copy link
Contributor

@maxwase maxwase commented May 27, 2021

This PR adds symlink method for the Path.

Tracking issue: #85748
For the discussion you can see internals topic

P.S.
I'm not fully sure about stable attribute, correct me if I'm wrong.

@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 @Mark-Simulacrum (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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 27, 2021
@m-ou-se
Copy link
Member

m-ou-se commented May 27, 2021

Did you test this on a regular file? This also returns true for regular files, on Unix at least.

@rust-log-analyzer

This comment has been minimized.

@m-ou-se
Copy link
Member

m-ou-se commented May 27, 2021

You'll have to look at the metadata itself (using .file_type().is_symlink()) to check if it's a symlink. symlink_metadata returns metadata about any file, not just symlinks. It just does not follow symlinks to allow you to get the metadata on symlinks too, unlike the regular metadata function.

@m-ou-se m-ou-se assigned m-ou-se and unassigned Mark-Simulacrum May 27, 2021
@m-ou-se m-ou-se added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 27, 2021
@m-ou-se
Copy link
Member

m-ou-se commented May 27, 2021

For the stability attribute: You'll have to come up with a new feature name (e.g. is_symlink or something).

Feel free to open a tracking issue for it.

For completeness, it's probably good to also add this function to std::fs::Metadata. That one also has both is_dir and is_file, but not is_symlink. You can do that in the same PR and in the same tracking issue, if you want.

@maxwase maxwase mentioned this pull request May 27, 2021
3 tasks
@maxwase
Copy link
Contributor Author

maxwase commented May 27, 2021

Thank you, @m-ou-se, for your remarks, I've changed implementation and added an issue.

Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

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

Thanks!

Looks good. I only have a few tiny comments on the documentation left:

library/std/src/fs.rs Outdated Show resolved Hide resolved
library/std/src/fs.rs Outdated Show resolved Hide resolved
library/std/src/path.rs Outdated Show resolved Hide resolved
library/std/src/path.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@maxwase maxwase requested a review from m-ou-se May 27, 2021 15:13
library/std/src/path.rs Outdated Show resolved Hide resolved
Co-authored-by: Mara Bos <m-ou.se@m-ou.se>
@maxwase maxwase requested a review from m-ou-se June 6, 2021 19:42
@m-ou-se
Copy link
Member

m-ou-se commented Jun 17, 2021

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Jun 17, 2021

📌 Commit f3c1db3 has been approved by m-ou-se

@bors bors 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 Jun 17, 2021
@bors
Copy link
Contributor

bors commented Jun 17, 2021

⌛ Testing commit f3c1db3 with merge 6ff500ed65c5e8c788456933c0e3ff983cfd7018...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 17, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 17, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Jun 17, 2021

Ah, the example doesn't compile on non-unix targets.

You could replace the /// ```no_run line with:

    #[cfg_attr(unix, doc = "```no_run")]
    #[cfg_attr(not(unix), doc = "```ignore")]

Here's an example:

/// # Examples
///
#[cfg_attr(any(target_os = "android", target_os = "linux"), doc = "```no_run")]
#[cfg_attr(not(any(target_os = "android", target_os = "linux")), doc = "```ignore")]
/// #![feature(unix_socket_ancillary_data)]
/// use std::os::unix::net::UnixStream;
///

@m-ou-se
Copy link
Member

m-ou-se commented Jun 18, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jun 18, 2021

📌 Commit 01435fc has been approved by m-ou-se

@bors bors 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 Jun 18, 2021
@bors
Copy link
Contributor

bors commented Jun 18, 2021

⌛ Testing commit 01435fc with merge 88ba8ad...

@bors
Copy link
Contributor

bors commented Jun 18, 2021

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 88ba8ad to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 18, 2021
@bors bors merged commit 88ba8ad into rust-lang:master Jun 18, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 18, 2021
bors added a commit to rust-lang/cargo that referenced this pull request Jun 21, 2021
Disambiguate is_symlink.

`Path::is_symlink` was added in rust-lang/rust#85747 which triggers the `unstable_name_collisions` lint, breaking Cargo's CI.  This switches it to a free function to avoid the collision.
ehuss pushed a commit to ehuss/cargo that referenced this pull request Jun 22, 2021
Disambiguate is_symlink.

`Path::is_symlink` was added in rust-lang/rust#85747 which triggers the `unstable_name_collisions` lint, breaking Cargo's CI.  This switches it to a free function to avoid the collision.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants