Improve docs in os::windows::ffi and os::windows::fs #41870

randomPoison · 2017-05-10T02:30:00Z

This PR makes changes to the documentation in os::windows::ffi and os::windows::fs with the goal of fleshing them out and bringing them in line with Rust's quality standards.

r? @steveklabnik

…-docs

rust-highfive · 2017-05-10T02:30:07Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (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.

TimNN · 2017-05-10T21:08:01Z

Hi @excaliburHisSheath and thanks for the PR! We'll periodically check in on it to make sure that @steveklabnik or someone else from the team reviews it soon.

Mark-Simulacrum · 2017-05-14T16:05:15Z

@steveklabnik Just a reminder that this is still looking for a review from you.

cc @frewsxcv

frewsxcv

this is great, thanks so much! a couple small comments, and this is good to go

frewsxcv · 2017-05-14T20:20:39Z

src/libstd/sys/windows/ext/ffi.rs

+    /// Re-encodes an `OsStr` as a wide character sequence, i.e. potentially
+    /// ill-formed UTF-16.
+    ///
+    /// This is lossless: calling [`OsString::from_wide()`] and then


very minor, but can you remove all the () parens attached to method/function names in these docs? for the time being, the current policy is to remove them in documentation. see also #40456

Ah, I was under the impression that the () parens were the desired style, thanks for clarifying that this isn't the case (I can update documentation for my own crates as well).

For my own curiosity, is there some discussion around why the decision was made to remove the () parens when referencing function in documentation? Personally I liked that style, so I'm curious as to why the change was made.

It wasn't that a change was made; it was that while proposing https://github.com/rust-lang/rfcs/blob/master/text/1574-more-api-documentation-conventions.md, we suggested that style, but it didn't make it through the process.

(I really like the ()s so I was just adding them in before we had an actual convention)

frewsxcv · 2017-05-14T21:00:23Z

src/libstd/sys/windows/ext/mod.rs

+//! Provides access to platform-level information for Windows, and exposes
+//! Windows-specific idioms that would otherwise be inappropriate as part
+//! the core `std` library. These extensions allow developers to use
+//! `std` types and idioms with Windows in a way that the noraml


typo: 'noraml'

frewsxcv · 2017-05-14T21:10:11Z

src/libstd/sys/windows/ext/fs.rs

+    ///
+    /// # Examples
+    ///
+    /// ```ignore


this example (and potentially other examples) could be no_run instead of ignore so they get compiled, but not run

Cool, I wasn't aware of the distinction. Is there a general guideline to when examples should be no_run vs ignore vs tested normally?

https://doc.rust-lang.org/book/documentation.html#running-documentation-tests

the short answer is, if it doesn't compile, ignore, and if it compiles but you don't want it to run (say for example, a network test), then no_run.

steveklabnik

This is great, thank you! Sorry for the delay here; I took two days at the end of last week off, so I'm a bit behind on reviews.

I agree with @frewsxcv 's comments, but have no other ones of my own. r=me after they're fixed 😄

- Remove `()` parens when referencing functions in docs. - Change some examples to be no_run instead of ignore. - Normalize style in examples for `OpenOptionsExt`. - Fix typo in windows mod docs.

randomPoison · 2017-05-19T02:15:46Z

@frewsxcv I believe I've addressed your concerns. I've left a number of tests no_run since they'll create a file foo.txt and it seems like bad practice to have tests create random files on disk. If it's preferred that we run the tests anyway I can change them, I think the tests still pass when run.

I've also made some changes to make the code style for the various OpenOptionsExt examples more consistent.

frewsxcv · 2017-05-19T03:06:55Z

Looks great, thanks!

@bors r+ rollup

bors · 2017-05-19T03:06:56Z

📌 Commit 4cd838b has been approved by frewsxcv

bors · 2017-05-19T12:27:35Z

⌛ Testing commit 4cd838b with merge 8d5a9ac...

randomPoison · 2017-05-19T14:19:35Z

@frewsxcv Heads up, I pushed another commit after you approved because I noticed a doc test was failing. It doesn't look like the automated tests have caught the failure yet, though. I don't know if you need to do anything to make sure the new commit gets included in the merge.

frewsxcv · 2017-05-19T14:56:20Z

@bors r+ rollup

bors · 2017-05-19T14:56:21Z

📌 Commit a892925 has been approved by frewsxcv

@steveklabnik

…-docs, r=frewsxcv Improve docs in os::windows::ffi and os::windows::fs Part of rust-lang#29367 This PR makes changes to the documentation in `os::windows::ffi` and `os::windows::fs` with the goal of fleshing them out and bringing them in line with Rust's quality standards. r? @steveklabnik

Rollup of 17 pull requests - Successful merges: #41870, #41910, #41958, #41971, #42006, #42024, #42037, #42056, #42067, #42070, #42079, #42080, #42082, #42089, #42092, #42096, #42100 - Failed merges:

randomPoison added 6 commits May 5, 2017 20:35

Update documention in windows::ffi

db16ca7

Update documentation in windows::ffi

e406cd1

Merge remote-tracking branch 'rust-lang/master' into iss29367-windows…

d7df6dc

…-docs

Update documentation in windows::fs

76128b3

Fix documentation tests in windows::fs

c350bc1

Fix tidy errors

5135cc8

rust-highfive assigned steveklabnik May 10, 2017

TimNN added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 10, 2017

frewsxcv assigned frewsxcv and unassigned steveklabnik May 14, 2017

frewsxcv suggested changes May 14, 2017

View reviewed changes

frewsxcv reviewed May 14, 2017

View reviewed changes

steveklabnik approved these changes May 15, 2017

View reviewed changes

arielb1 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 May 16, 2017

Normalize docs in windows::ffi and windows::fs

4cd838b

- Remove `()` parens when referencing functions in docs. - Change some examples to be no_run instead of ignore. - Normalize style in examples for `OpenOptionsExt`. - Fix typo in windows mod docs.

frewsxcv approved these changes May 19, 2017

View reviewed changes

Fix doc test failure for OpenOptionsExt

a892925

Mark-Simulacrum mentioned this pull request May 19, 2017

Rollup of 17 pull requests #42105

Merged

bors merged commit a892925 into rust-lang:master May 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve docs in os::windows::ffi and os::windows::fs #41870

Improve docs in os::windows::ffi and os::windows::fs #41870

randomPoison commented May 10, 2017

rust-highfive commented May 10, 2017

TimNN commented May 10, 2017

Mark-Simulacrum commented May 14, 2017

frewsxcv left a comment

frewsxcv May 14, 2017

randomPoison May 14, 2017

randomPoison May 14, 2017

steveklabnik May 15, 2017

frewsxcv May 14, 2017

frewsxcv May 14, 2017

randomPoison May 14, 2017

frewsxcv May 14, 2017

steveklabnik May 15, 2017

steveklabnik left a comment

randomPoison commented May 19, 2017

frewsxcv commented May 19, 2017

bors commented May 19, 2017

bors commented May 19, 2017

randomPoison commented May 19, 2017

frewsxcv commented May 19, 2017

bors commented May 19, 2017

Improve docs in os::windows::ffi and os::windows::fs #41870

Improve docs in os::windows::ffi and os::windows::fs #41870

Conversation

randomPoison commented May 10, 2017

rust-highfive commented May 10, 2017

TimNN commented May 10, 2017

Mark-Simulacrum commented May 14, 2017

frewsxcv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklabnik left a comment

Choose a reason for hiding this comment

randomPoison commented May 19, 2017

frewsxcv commented May 19, 2017

bors commented May 19, 2017

bors commented May 19, 2017

randomPoison commented May 19, 2017

frewsxcv commented May 19, 2017

bors commented May 19, 2017