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

Docs: add corner cases of Path::file_name() #88044

Closed
wants to merge 1 commit into from

Conversation

marcospb19
Copy link
Contributor

Documenting corner cases where Path::file_name() returns None, and adding examples.

@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.

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 Aug 15, 2021
@marcospb19
Copy link
Contributor Author

marcospb19 commented Aug 15, 2021

Here's the page rendered:

image

This markdown list is looking kinda squished and confusing, maybe I should just throw these corner cases in the doc test assertions? And remove the "(e.g. ...)" examples.

Copy link
Contributor

@jfrimmel jfrimmel left a comment

Choose a reason for hiding this comment

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

Maybe this isbbetter (remove quotes in favor of only markdown code blocks)?

library/std/src/path.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
@marcospb19
Copy link
Contributor Author

@jfrimmel I do think it looks weird:
image

"Terminates in ..." looks like 3 dots, instead of 2.

I will also add "(double dots)" to the side of it, to see if it helps.

@marcospb19
Copy link
Contributor Author

Here's the current status:
image

/// - Is empty.
/// - Terminates in `..` (double dots).
/// - Is root (e.g. `/`, `/.`, `/././.`).
/// - Only contains current directory (e.g. `.`, `././.`).
Copy link
Member

Choose a reason for hiding this comment

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

I think this is missing the Prefix variant of Component -- maybe it is more useful to say "returns None if the last component of the path is not Normal" and link to https://doc.rust-lang.org/nightly/std/path/enum.Component.html?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great idea, however, here is the documentation of Component:

image

It does not explain that running components on "a/." gives Component::Normal("a") instead of Normal("a") AND CurrrentDirectory, plus I think people are expecting "asd/.".file_name() to return ".", so, maybe this should be explained somewhere?

Maybe I could add examples to the docs of Component itself to show what iterating on Path::new("/a/b/c") retrieves, then we could also cover this corner cases.

However, there is already something like this at https://doc.rust-lang.org/nightly/std/path/struct.Path.html#method.components.

use std::path::{Path, Component};
use std::ffi::OsStr;

let mut components = Path::new("/tmp/foo.txt").components();

assert_eq!(components.next(), Some(Component::RootDir));
assert_eq!(components.next(), Some(Component::Normal(OsStr::new("tmp"))));
assert_eq!(components.next(), Some(Component::Normal(OsStr::new("foo.txt"))));
assert_eq!(components.next(), None)

So, maybe I should add more examples to path::components instead? Or add some explanation in the Component page itself.

Waiting for some feedback on these thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

I think adding more docs to Component makes sense, since it's a dedicated page for this. We can point there from file_name() and potentially components() as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, I agree! I've been getting very busy this days, so I might need to delay this PR just a couple weeks, but thanks for the review.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 19, 2021
@marcospb19 marcospb19 marked this pull request as draft August 29, 2021 19:35
@the8472 the8472 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 8, 2021
@Dylan-DPC
Copy link
Member

@marcospb19 any updates on this?

@marcospb19
Copy link
Contributor Author

@Mark-Simulacrum @Dylan-DPC sorry for the huge delay on this, I'm struggling with managing my personal time.

Was not able to finish this yet.

If someone want to open a PR with it finished, I'd be glad, you might want to close mine.

(Or commit on my branch, for some repo member).

@Dylan-DPC
Copy link
Member

@marcospb19 that's totally fine and understandable. I'll close this for now, thanks for taking the time to contribute to rust.

@Dylan-DPC Dylan-DPC closed this Feb 24, 2022
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants