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

Improve the documentation for std::convert (From, Into, AsRef and AsMut) #59330

Merged
merged 7 commits into from Mar 27, 2019

Conversation

DevQps
Copy link
Contributor

@DevQps DevQps commented Mar 20, 2019

Description

In this PR I updated the documentation of From, Into, AsRef and AsMut, as well as the general std::convert module documentation. The discussion in #59163 provided information that was not yet present in the docs, or was not expressed clearly enough. I tried to clarify the examples that were already present in the docs as well as add more information about considered best-practices that came out of the discussion in #59163

@steveklabnik I hope I didn't change too much. This is an initial version! I will scan through everything tomorrow as well again to see if I made any typo's or errors, and maybe make some small changes here and there.

All suggestions are welcome!

closes #59163

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 20, 2019
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:014fda7c:start=1553120794077718353,finish=1553120879772457804,duration=85694739451
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
Setting environment variables from .travis.yml
---

[00:03:16] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:16] tidy error: /checkout/src/libcore/convert.rs:9: line longer than 100 chars
[00:03:16] tidy error: /checkout/src/libcore/convert.rs:10: line longer than 100 chars
[00:03:16] tidy error: /checkout/src/libcore/convert.rs:199: line longer than 100 chars
[00:03:16] tidy error: /checkout/src/libcore/convert.rs:200: line longer than 100 chars
[00:03:16] tidy error: /checkout/src/libcore/convert.rs:201: line longer than 100 chars
[00:03:16] tidy error: /checkout/src/libcore/convert.rs:212: line longer than 100 chars
[00:03:16] tidy error: /checkout/src/libcore/convert.rs:236: line longer than 100 chars
[00:03:16] tidy error: /checkout/src/libcore/convert.rs:237: line longer than 100 chars
[00:03:16] tidy error: /checkout/src/libcore/convert.rs:277: line longer than 100 chars
[00:03:16] tidy error: /checkout/src/libcore/convert.rs:278: line longer than 100 chars
[00:03:16] tidy error: /checkout/src/libcore/convert.rs:311: line longer than 100 chars
[00:03:16] tidy error: /checkout/src/libcore/convert.rs:312: line longer than 100 chars
[00:03:16] tidy error: /checkout/src/libcore/convert.rs:313: line longer than 100 chars
[00:03:17] some tidy checks failed
[00:03:17] 
[00:03:17] 
[00:03:17] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:17] 
[00:03:17] 
[00:03:17] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:17] Build completed unsuccessfully in 0:00:44
[00:03:17] Build completed unsuccessfully in 0:00:44
[00:03:17] Makefile:67: recipe for target 'tidy' failed
[00:03:17] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0d4c406a
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Wed Mar 20 22:31:26 UTC 2019
---
travis_time:end:2d247e6b:start=1553121086771121895,finish=1553121086776201799,duration=5079904
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1fc13f5e
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:00fe8140
travis_time:start:00fe8140
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:208c33f2
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Copy link
Contributor

@tesuji tesuji left a comment

Choose a reason for hiding this comment

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

Some nits on indentations.

src/libcore/convert.rs Outdated Show resolved Hide resolved
src/libcore/convert.rs Outdated Show resolved Hide resolved
@steveklabnik
Copy link
Member

Hey hey! I won't have time until tomorrow to review this; @rust-lang/docs can you check it out in the meantime?

Thanks so much!

src/libcore/convert.rs Outdated Show resolved Hide resolved
src/libcore/convert.rs Outdated Show resolved Hide resolved
@tesuji
Copy link
Contributor

tesuji commented Mar 21, 2019

One of the thing I find that it's hard to review docs PR because we don't have
visual built documentation of the changes right away.

Building whole rust documentation for small changes is overkill and it is so slow
to build for regular doc contributor.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:1698e85c:start=1553190633769100723,finish=1553190754425390118,duration=120656289395
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
Setting environment variables from .travis.yml
---

[00:03:36] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:36] tidy error: /checkout/src/libcore/convert.rs:294: line longer than 100 chars
[00:03:38] some tidy checks failed
[00:03:38] 
[00:03:38] 
[00:03:38] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:38] 
[00:03:38] 
[00:03:38] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:38] Build completed unsuccessfully in 0:00:48
[00:03:38] Build completed unsuccessfully in 0:00:48
[00:03:38] make: *** [tidy] Error 1
[00:03:38] Makefile:67: recipe for target 'tidy' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1d036a5e
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Thu Mar 21 17:56:23 UTC 2019
---
travis_time:end:10f1c4b2:start=1553190985003954379,finish=1553190985010008759,duration=6054380
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0a8fde48
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:101a45c2
travis_time:start:101a45c2
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:045e0d2c
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@DevQps
Copy link
Contributor Author

DevQps commented Mar 21, 2019

@lzutao I can provide a tar or zip of the generated docs somehow but that isn't really ideal of course. Maybe it can be added as a downloadable asset at each Travis CI pipeline? I used to do that with Gitlab pipelines, not sure if Travis can do such a thing as well though.

@DevQps
Copy link
Contributor Author

DevQps commented Mar 25, 2019

@ehuss @lzutao @steveklabnik Let me know if there's anything else I can do to improve things!

@tesuji
Copy link
Contributor

tesuji commented Mar 25, 2019

This PR needs an review from Documentation team I think. You could ping any of them for requesting review.

@steveklabnik
Copy link
Member

I'm planning on reviewing within the next few hours.

Copy link
Member

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

Hey hey! This PR is great content-wise, I just have some formatting stuff. Please let me know if any of that isn't clear! As soon as the formatting is fixed, we can merge this in 👍

src/libcore/convert.rs Show resolved Hide resolved
src/libcore/convert.rs Show resolved Hide resolved
src/libcore/convert.rs Show resolved Hide resolved
src/libcore/convert.rs Show resolved Hide resolved
src/libcore/convert.rs Show resolved Hide resolved
src/libcore/convert.rs Show resolved Hide resolved
src/libcore/convert.rs Show resolved Hide resolved
src/libcore/convert.rs Show resolved Hide resolved
src/libcore/convert.rs Outdated Show resolved Hide resolved
src/libcore/convert.rs Outdated Show resolved Hide resolved
@DevQps
Copy link
Contributor Author

DevQps commented Mar 25, 2019

Hey hey! This PR is great content-wise, I just have some formatting stuff. Please let me know if any of that isn't clear! As soon as the formatting is fixed, we can merge this in 👍

@steveklabnik Thanks for your kind words! I fixed all the issues you brought up, thanks a lot for all the feedback :) Many of the things I was unaware of / didn't know so it will probably help me when I do some editing in the future as well.

I was wondering!: Are there are other things I could maybe help with? I am not sure if there are still documents to be reviewed? Or if there is some documentation that needs a test read / that needs some improvement? Now that I got the hang of it a little bit I would enjoy contributing some more! Let me know if there's anything I can do to help :)

EDIT: @lzutao @ehuss Thank you guys as well!

@steveklabnik
Copy link
Member

Awesome, thanks!

@bors: r+ rollup

I was wondering!: Are there are other things I could maybe help with? I am not sure if there are still documents to be reviewed? Or if there is some documentation that needs a test read / that needs some improvement? Now that I got the hang of it a little bit I would enjoy contributing some more! Let me know if there's anything I can do to help :)

For sure! We have a T-docs tag with issues. Picking out one of them and writing some docs would be fantastic!

@bors
Copy link
Contributor

bors commented Mar 26, 2019

📌 Commit 6c479c3 has been approved by steveklabnik

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 26, 2019
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Mar 26, 2019
…ation, r=steveklabnik

Improve the documentation for std::convert (From, Into, AsRef and AsMut)

# Description
In this PR I updated the documentation of From, Into, AsRef and AsMut, as well as the general std::convert module documentation. The discussion in rust-lang#59163 provided information that was not yet present in the docs, or was not expressed clearly enough. I tried to clarify the examples that were already present in the docs as well as add more information about considered best-practices that came out of the discussion in rust-lang#59163

@steveklabnik I hope I didn't change too much. This is an initial version! I will scan through everything tomorrow as well again to see if I made any typo's or errors, and maybe make some small changes here and there.

All suggestions are welcome!

closes rust-lang#59163
bors added a commit that referenced this pull request Mar 26, 2019
Rollup of 7 pull requests

Successful merges:

 - #59004 ([rustdoc] Improve "in parameters" search and search more generally)
 - #59026 (Fix moving text in search tabs headers)
 - #59197 (Exclude old book redirect stubs from search engines)
 - #59330 (Improve the documentation for std::convert (From, Into, AsRef and AsMut))
 - #59424 (Fix code block display in portability element in dark theme)
 - #59427 (Link to PhantomData in NonNull documentation)
 - #59432 (Improve some compiletest documentation)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Mar 27, 2019
Rollup of 7 pull requests

Successful merges:

 - #59004 ([rustdoc] Improve "in parameters" search and search more generally)
 - #59026 (Fix moving text in search tabs headers)
 - #59197 (Exclude old book redirect stubs from search engines)
 - #59330 (Improve the documentation for std::convert (From, Into, AsRef and AsMut))
 - #59424 (Fix code block display in portability element in dark theme)
 - #59427 (Link to PhantomData in NonNull documentation)
 - #59432 (Improve some compiletest documentation)

Failed merges:

r? @ghost
@bors bors merged commit 6c479c3 into rust-lang:master Mar 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the documentation of From and Into traits coherent.
7 participants