-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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 examples for GenericPath methods. #16113
Conversation
/// | ||
/// ``` | ||
/// let p = Path::new("abc/def"); | ||
/// assert_eq!(p.as_vec(), &[97, 98, 99, 47, 100, 101, 102]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that paths are internally normalized, so this test (and ones below) may fail on windows due to /
being normalized to \
. I don't think we currently run doc tests on windows, but this may surprise someone at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct. This is going to affect all of the examples that test the value of the path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the fix here to use PosixPath
instead of Path
? Or does that affect anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use path::SEP
, but I would avoid PosixPath
in the GenericPath
documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you could document at the module level that all the examples are written assuming unix?
@alexcrichton Do we have a tag for code blocks to skip them under Windows? You said we don't run doc tests on Windows right now but presumably that will change at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could sneak around it like this:
/// ```
/// #[cfg(not(windows))]
/// # fn foo() {
/// ... dox dox dox
///
/// ... dox dox dox
/// # }
/// # #[cfg(windows)] fn foo() {}
/// # foo()
/// ```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like a lot of effort to put in each example.
…in the last commit.
I left a now-hidden comment, but an icky but hidden-from-the-user strategy for dealing with windows may be something like this:
|
As for using
It's not horrible, but seems a bit icky. Alternatively, could I use |
It'd be some extra boilerplate required for the cfg(windows) or not, but in terms of documentation I think it's higher quality than |
I just attempted to implement skipping the doc tests for windows. Hopefully I've done that correctly |
…ykril fix: self type replacement in inline-function Fix rust-lang#16113, fix rust-lang#16091 The problem described in this issue actually involves three bugs. Firstly, when using `ted` to modify the syntax tree, the offset of nodes on the tree changes, which causes the syntax range information from `hir` to become invalid. Therefore, we need to edit the AST after the last usage for `usages_for_locals`. The second issue is that when inserting nodes, it's necessary to use `clone_subtree` for duplication because the `ted::replace` operation essentially moves a node. The third issue is that we should use `ancestors_with_macros` instead of `ancestors` to handle impl definition in macros. I have fixed the three bugs mentioned above and added unit tests.
I wanted to use
assert_eq
throughout, but that does not seem possible (see #15625)