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

Added platform notes to std::fs public functions. #29732

Merged
merged 3 commits into from
Jan 4, 2016
Merged

Added platform notes to std::fs public functions. #29732

merged 3 commits into from
Jan 4, 2016

Conversation

nathansizemore
Copy link
Contributor

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

///
/// This function currently corresponds to the `unlink` function on Unix
/// and the `DeleteFile` function on Windows. Note that, this
/// [may change in the future.][https://github.com/rust-lang/rust/pull/28613]
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. One of the things about this PR is that we need to figure out the standard way to encode this kind of thing, I guess a link to github would be reasonable.

Two nits here: 1: this needs to be []() not [][], and the period should be outside the link. However, we prefer the reference style links, like

/// [may change in the future][changes].
///
/// [changes]: url 

So that should be done, regardless.

@steveklabnik
Copy link
Member

Thank you so much for taking this on! A few nits, and I'd like @rust-lang/libs to take a look as well.

@steveklabnik
Copy link
Member

It might be worth adding this new standard header to https://doc.rust-lang.org/book/documentation.html#special-sections too

@nathansizemore
Copy link
Contributor Author

Whoops, guess I got in a hurry and totally over looked placing the links at the bottom :(
I'll get this stuff edited and push up again

@steveklabnik
Copy link
Member

it's all good! This is exactly why we do review :)

@nathansizemore
Copy link
Contributor Author

Once there has been enough discussion and decision on what the title for the section actually should be, I'll get it into the special sections too.

@alexcrichton
Copy link
Member

Looking good to me! I like @steveklabnik's idea about "Platform-specific behavior" as a heading, and I think that this may also want to just have a dedicated section of the std::io docs or something like that as a URL for reference (rather than the previous PR). For example something like:

Platform-specific behavior

Many I/O functions throughout the standard library are documented to indicate what various library or syscalls they are delegated to. This is done to help applications both understand what's happening under the hood as well as investigate any possibly unclear semantics. Note, however, that this is informative, not a binding contract. The implementation of many of these functions are subject to change over time and may call fewer or more syscalls/library functions.

@bors
Copy link
Contributor

bors commented Dec 28, 2015

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

@steveklabnik
Copy link
Member

@nathansizemore sorry, github doesn't update us when a commit gets pushed, so i missed this! There's merge conficts now, so would you mind rebasing, and then we can land this? Thanks!

@steveklabnik steveklabnik assigned steveklabnik and unassigned aturon Dec 31, 2015
@nathansizemore
Copy link
Contributor Author

@steveklabnik should be ready to go!

@steveklabnik
Copy link
Member

@bors: r+ rollup

Thanks so much!

@bors
Copy link
Contributor

bors commented Jan 3, 2016

📌 Commit 3e9d5fe has been approved by steveklabnik

@bors
Copy link
Contributor

bors commented Jan 3, 2016

⌛ Testing commit 3e9d5fe with merge 720afe5...

@bors
Copy link
Contributor

bors commented Jan 3, 2016

💔 Test failed - auto-linux-64-nopt-t

@sfackler
Copy link
Member

sfackler commented Jan 3, 2016

@bors retry

@bors
Copy link
Contributor

bors commented Jan 4, 2016

⌛ Testing commit 3e9d5fe with merge 99e59de...

@bors bors merged commit 3e9d5fe into rust-lang:master Jan 4, 2016
@aturon
Copy link
Member

aturon commented Jan 5, 2016

\o/

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.

7 participants