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

create method overview docs for core::option and core::result #86211

Merged
merged 12 commits into from
Jul 14, 2021

Conversation

tlyu
Copy link
Contributor

@tlyu tlyu commented Jun 11, 2021

The Option and Result types have large lists of methods. They each could use an overview page of methods grouped by category. These proposed overviews include "truth tables" for the underappreciated boolean operators/combinators of these types. The methods are already somewhat categorized in the source, but some logical groupings are broken up by the necessities of putting related methods in different impl blocks, for example.

This is based on #86209, but those are small changes and unlikely to conflict.

tlyu added 3 commits June 10, 2021 22:30
Fix some awkward wording in the `core::option` documentation in the
"Options and pointers" section.
Fix a typo/missed replacement in the documentation for
`impl From<&Option<T>> for Option<&T>` in `core::option`.
@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 11, 2021
@tlyu
Copy link
Contributor Author

tlyu commented Jun 11, 2021

This is still a work in progress. I wanted to get something out there for early feedback before I got too far into it. The Option overview is more complete than the one for Result. The similarity between the types would make it somewhat easy to copy parts of the work from one overview to the other.

tlyu added 3 commits June 11, 2021 11:34
Fix an error in `map_or_else`. Use more descriptive text for
"don't care" in truth tables. Make minor corrections to truth tables.
Rename `makeiter` to `make_iter` in examples.
Copy link
Member

@yaahc yaahc left a comment

Choose a reason for hiding this comment

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

This looks amazing btw, thank you for putting it together.

library/core/src/option.rs Outdated Show resolved Hide resolved
library/core/src/option.rs Show resolved Hide resolved
library/core/src/option.rs Outdated Show resolved Hide resolved
library/core/src/option.rs Outdated Show resolved Hide resolved
library/core/src/option.rs Show resolved Hide resolved
library/core/src/option.rs Outdated Show resolved Hide resolved
library/core/src/option.rs Outdated Show resolved Hide resolved
library/core/src/option.rs Outdated Show resolved Hide resolved
library/core/src/result.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

Make minor wording changes in a few places. Move `filter` to the
"transformations" section. Add `zip` methods to the "transformations"
section. Clarify the section about `Option` iterators, and add a section
about collecting into `Option`.

Clarify that for `Result`, `or` and `or_else` can also produce a
`Result` having a different type.
@tlyu
Copy link
Contributor Author

tlyu commented Jun 22, 2021

I think this is about ready for review. I think the overviews cover most of the methods of interest, but aren't exhaustive. The main other thing I might want to do is to take @yaahc's suggestion about reordering the lazy-evaluated boolean methods first, and explain the short-circuiting in more detail. But maybe that could be a separate PR?

@rustbot label +A-docs +T-libs

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 22, 2021
@tlyu tlyu marked this pull request as ready for review June 22, 2021 20:55
@rustbot rustbot added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Jun 22, 2021
(Most of these are from a review by joshtriplett. Thanks!)

Fix errors in `as_pin_ref` and `as_pin_mut` in the "Adapters for
working with references" overview.

Reword some headings about transformation methods.

Reclassify `map`, `map_or`, `map_or_else`, `map_err`, etc. to more
accurately reflect which variants they transform.

Document `Debug` requirement for `get_or_insert_default`.

Reword text about `take` and `replace` to be more accurate.

Add examples for the `Product` and `Sum` traits.

Also:

Move link reference definitions closer to their uses. Warn about making
link reference definintions for `err` and `ok`. Avoid making other link
reference definitions that might conflict in the future (foreign methods
that share a name with local ones, etc.)

Write out the generics of `Option` and `Result` when the following
text refers to the type parameters.
@tlyu
Copy link
Contributor Author

tlyu commented Jun 24, 2021

Thanks for the review! I've pushed an update that should address these. (Also fixed a few other small things I noticed along the way.) Please let me know what you think.

@yaahc
Copy link
Member

yaahc commented Jul 1, 2021

@tlyu could you mark the comments as resolved that you've already dealt with? Otherwise it's hard to know which comments if any still need attention.

@tlyu
Copy link
Contributor Author

tlyu commented Jul 2, 2021

@tlyu could you mark the comments as resolved that you've already dealt with? Otherwise it's hard to know which comments if any still need attention.

Thanks! Done. I also pushed a minor update to add a missing word that I noticed while re-reviewing the comments.

@yaahc
Copy link
Member

yaahc commented Jul 6, 2021

Awesome, I went ahead and resolved the last two comments which you'd given responses to that I felt happy with upon review. I'm happy with this PR and ready to r+ it right now assuming @joshtriplett is also happy with it. Thank you again for this wonderfully detailed addition @tlyu!

@joshtriplett
Copy link
Member

Looks great, thank you! One very minor nit, and otherwise r=me.

@jyn514
Copy link
Member

jyn514 commented Jul 14, 2021

@bors r=joshtriplett

@bors
Copy link
Contributor

bors commented Jul 14, 2021

📌 Commit 2b4a6aa has been approved by joshtriplett

@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 Jul 14, 2021
@bors
Copy link
Contributor

bors commented Jul 14, 2021

⌛ Testing commit 2b4a6aa with merge a08f25a...

@bors
Copy link
Contributor

bors commented Jul 14, 2021

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing a08f25a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 14, 2021
@bors bors merged commit a08f25a into rust-lang:master Jul 14, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 14, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 17, 2021
fix typo in compile_fail doctest

Fixes a typo introduced by rust-lang#86211. For some reason this typo makes Miri go all crazy when running libcore doctests (rust-lang/miri#1852). Kudos to `@hyd-dev` for noticing the typo.

Cc `@tlyu` `@joshtriplett`
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 18, 2021
fix typo in compile_fail doctest

Fixes a typo introduced by rust-lang#86211. For some reason this typo makes Miri go all crazy when running libcore doctests (rust-lang/miri#1852). Kudos to ``@hyd-dev`` for noticing the typo.

Cc ``@tlyu`` ``@joshtriplett``
@tlyu tlyu deleted the option-result-overviews branch July 22, 2021 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.