API docs: Convert #40987

maccoda · 2017-04-01T06:11:48Z

Clean up of the convert module documentation following points in #29349

rust-highfive · 2017-04-01T06:12:02Z

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

maccoda · 2017-04-01T06:13:18Z

r? @steveklabnik

frewsxcv · 2017-04-01T23:38:11Z

Looks like our tidy checker wants some lines wrapped:

[00:02:33] /checkout/src/libcore/convert.rs:59: line longer than 100 chars
[00:02:33] /checkout/src/libcore/convert.rs:60: line longer than 100 chars
[00:02:33] /checkout/src/libcore/convert.rs:64: line longer than 100 chars
[00:02:33] /checkout/src/libcore/convert.rs:164: line longer than 100 chars
[00:02:33] /checkout/src/libcore/convert.rs:207: line longer than 100 chars
[00:02:33] /checkout/src/libcore/convert.rs:209: line longer than 100 chars
[00:02:33] /checkout/src/libcore/convert.rs:210: line longer than 100 chars
[00:02:33] /checkout/src/libcore/convert.rs:212: line longer than 100 chars

maccoda · 2017-04-02T21:59:52Z

Thanks just confirmed with @steveklabnik that want standard library docs wrapped to 80 characters so that should address that 😄

maccoda · 2017-04-02T23:58:43Z

Broken links this time will fix next chance I get.

[00:41:01] Linkcheck (x86_64-unknown-linux-gnu)
[00:41:05] std/convert/trait.AsRef.html:54: broken link - std/struct.Path.html
[00:41:05] std/convert/trait.From.html:54: broken link fragment  `#the-from-trait` pointing to `book/error-handling.html`
[00:41:06] core/convert/trait.AsRef.html:54: broken link - std/struct.Path.html
[00:41:06] core/convert/trait.From.html:54: broken link fragment  `#the-from-trait` pointing to `book/error-handling.html`

maccoda · 2017-04-03T10:57:17Z

Closing until I get a clean build, have realized not yet ready.

carols10cents · 2017-04-17T14:25:18Z

ping @steveklabnik @frewsxcv -- looks like this is passing and ready for r now?

steveklabnik

thanks for this! Looks great overall but there's some formatting details to take care of. We're good to go after they're fixed.

steveklabnik · 2017-04-17T15:11:20Z

src/libcore/convert.rs

-//! - [`From`] and [`Into`] are reflexive, which means that all types can `into()`
-//!   themselves and `from()` themselves
+//! - [`From`] and [`Into`] are reflexive, which means that all types can
+//!   `into()` themselves and `from()` themselves


very minor nit: the ()s shouldn't be here. Not your fault, but while you're here, fixing them would be nice, on both into and from

Not a problem, boy scout rule applied 🤓

steveklabnik · 2017-04-17T15:13:01Z

src/libcore/convert.rs

@@ -50,20 +50,41 @@

 use str::FromStr;

-/// A cheap, reference-to-reference conversion.
+/// A cheap reference-to-reference conversion. Used to convert a value to a
+/// reference value within generic code.


I wonder if this isn't too long for a summary line; it might get cut off on https://doc.rust-lang.org/stable/std/convert/index.html. Have you checked? if not, maybe

A cheap reference-to-reference conversion, used within generic code.

Have run the rustdoc build and appears to be all fine.

steveklabnik · 2017-04-17T15:13:29Z

src/libcore/convert.rs

+/// useful when wishing to abstract
+/// over the type of reference (`&T`, `&mut T`) or allow both the referenced
+/// and owned type to be treated in the same manner.
+/// The key difference between the two traits is the intention:


could you put a /// above this line?

steveklabnik · 2017-04-17T15:13:58Z

src/libcore/convert.rs

+/// `Borrow` is more related to the notion of taking the reference. It is
+/// useful when wishing to abstract
+/// over the type of reference (`&T`, `&mut T`) or allow both the referenced
+/// and owned type to be treated in the same manner.


everything above here is treated as one paragraph in markdown, but are broken up strangely in the text, could you reflow this paragraph please?

Ah don't know what happened here. Reshaped into a clearer single paragraph.

steveklabnik · 2017-04-17T15:14:20Z

src/libcore/convert.rs

+///
+/// - Use `AsRef` when goal is to simply convert into a reference
+/// - Use `Borrow` when goal is related to writing code that is agnostic to the
+/// type of borrow and if is reference or value


i think this needs to be indented to match up with the Use above; not 100% sure though

Looks neater anyway so will indent them, it appeared to render fine though 😄

steveklabnik · 2017-04-17T15:14:39Z

src/libcore/convert.rs

+/// # Generic Implementations
+///
+/// - `AsRef` auto-dereferences if the inner type is a reference or a mutable
+/// reference (e.g.: `foo.as_ref()` will work the same if `foo` has type


same here, dunno if these lines need to be indented

steveklabnik · 2017-04-17T15:14:51Z

src/libcore/convert.rs

+/// # Generic Implementations
+///
+/// - `AsMut` auto-dereferences if the inner type is a reference or a mutable
+/// reference (e.g.: `foo.as_ref()` will work the same if `foo` has type


And indented again 😄

steveklabnik · 2017-04-17T15:15:32Z

src/libcore/convert.rs

+/// erroneous situations.
+/// This trait is not limited to error handling, rather the general case for
+/// this trait would be in any type conversions to have an explicit definition
+/// of how they are performed.


these three paragraphs will get smooshed into one by markdown; could you add /// lines between them?

Yes my bad. Now separated.

steveklabnik · 2017-04-17T15:16:02Z

src/libcore/convert.rs

@@ -236,15 +310,19 @@ pub trait TryFrom<T>: Sized {

 // As lifts over &
 #[stable(feature = "rust1", since = "1.0.0")]
-impl<'a, T: ?Sized, U: ?Sized> AsRef<U> for &'a T where T: AsRef<U> {
+impl<'a, T: ?Sized, U: ?Sized> AsRef<U> for &'a T


all the changes from here below are re-styling code and probably don't belong here

Ah formatter got the best of me. Reverting these changes.

alexcrichton · 2017-04-20T15:06:07Z

Looks like this was updated, mind taking another look @steveklabnik and/or @frewsxcv?

@maccoda also when you update a PR feel free to ping it, unfortunately github doesn't send out notifications for updating a PR :(

steveklabnik · 2017-04-20T15:36:54Z

LGTM, thanks @maccoda

@bors: r+ rollup

bors · 2017-04-20T15:36:55Z

📌 Commit 2877a01 has been approved by steveklabnik

API docs: Convert Clean up of the convert module documentation following points in rust-lang#29349

Rollup of 2 pull requests - Successful merges: #40812, #40987 - Failed merges:

maccoda · 2017-04-20T22:20:12Z

@alexcrichton thank you for that didn't know didn't send notifications will remember to do this next time!

@steveklabnik thank you, look forward to working on the next one

Convert docs clean up.

7897e16

rust-highfive assigned brson Apr 1, 2017

rust-highfive assigned steveklabnik and unassigned brson Apr 1, 2017

frewsxcv added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Apr 1, 2017

Wrapped to 80 characters. Fix links.

79efca1

maccoda force-pushed the convert-docs branch from 544ff7e to 79efca1 Compare April 3, 2017 08:58

maccoda closed this Apr 3, 2017

AsMut example

bb84746

maccoda reopened this Apr 5, 2017

Removing broken examples

d35fa1e

maccoda force-pushed the convert-docs branch from 498f325 to d35fa1e Compare April 7, 2017 23:33

Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 14, 2017

steveklabnik suggested changes Apr 17, 2017

View reviewed changes

Address review comments

2877a01

steveklabnik approved these changes Apr 20, 2017

View reviewed changes

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 20, 2017

Rollup merge of rust-lang#40987 - maccoda:convert-docs, r=steveklabnik

9d91822

API docs: Convert Clean up of the convert module documentation following points in rust-lang#29349

frewsxcv mentioned this pull request Apr 20, 2017

Rollup of 2 pull requests #41424

Merged

bors added a commit that referenced this pull request Apr 20, 2017

Auto merge of #41424 - frewsxcv:rollup, r=frewsxcv

ddc5d7b

Rollup of 2 pull requests - Successful merges: #40812, #40987 - Failed merges:

bors merged commit 2877a01 into rust-lang:master Apr 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API docs: Convert #40987

API docs: Convert #40987

maccoda commented Apr 1, 2017

rust-highfive commented Apr 1, 2017

maccoda commented Apr 1, 2017

frewsxcv commented Apr 1, 2017

maccoda commented Apr 2, 2017

maccoda commented Apr 2, 2017

maccoda commented Apr 3, 2017

carols10cents commented Apr 17, 2017

steveklabnik left a comment

steveklabnik Apr 17, 2017

maccoda Apr 17, 2017

steveklabnik Apr 17, 2017

maccoda Apr 17, 2017

steveklabnik Apr 17, 2017

maccoda Apr 17, 2017

steveklabnik Apr 17, 2017

maccoda Apr 17, 2017

steveklabnik Apr 17, 2017

maccoda Apr 17, 2017

steveklabnik Apr 17, 2017

maccoda Apr 17, 2017

steveklabnik Apr 17, 2017

maccoda Apr 17, 2017

steveklabnik Apr 17, 2017

maccoda Apr 17, 2017

steveklabnik Apr 17, 2017

maccoda Apr 17, 2017

alexcrichton commented Apr 20, 2017

steveklabnik commented Apr 20, 2017

bors commented Apr 20, 2017

maccoda commented Apr 20, 2017

API docs: Convert #40987

API docs: Convert #40987

Conversation

maccoda commented Apr 1, 2017

rust-highfive commented Apr 1, 2017

maccoda commented Apr 1, 2017

frewsxcv commented Apr 1, 2017

maccoda commented Apr 2, 2017

maccoda commented Apr 2, 2017

maccoda commented Apr 3, 2017

carols10cents commented Apr 17, 2017

steveklabnik 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

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

alexcrichton commented Apr 20, 2017

steveklabnik commented Apr 20, 2017

bors commented Apr 20, 2017

maccoda commented Apr 20, 2017