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.

Sign up for GitHub

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

Explain meaning of Result iters and link to factory functions #38158

Merged
merged 1 commit into from
Dec 21, 2016
Merged

Explain meaning of Result iters and link to factory functions #38158

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions src/libcore/result.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,8 @@ impl<T, E> Result<T, E> {

/// Returns an iterator over the possibly contained value.
///
/// The iterator yields one value if the result is [`Ok`], otherwise none.
Copy link
Member

Choose a reason for hiding this comment

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

Otherwise None (with url).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, no, it doesn't yield None, it just yields no values at all.

I realize this is implemented by next() returning None but I think saying that would only make this confusing. I could rephrase it as "the iterator's next method returns Some of the inner and then None if the result is Ok, otherwise it returns None." But that seems to be needlessly puncturing the iterator abstraction.

A couple of general comments:

  • There are lots of references to well-known types such as None and Ok throughout the code without hyperlinks, indeed even in this very file. If the goal is that every type be manually linked maybe that should go into CONTRIBUTING.md to avoid make that clear. IMO it is not so necessary to linkify values the reader is highly likely to already understand, and that anyhow are linked from nearby.

  • In my experience repeatedly asking contributors for small incremental changes to a PR that's already an improvement tends to discourage future contributions. My normal practice now is to accept the change and encourage them to send another PR making additional cleanups, and I think this gives them a better feeling about contributing.

Copy link
Member

@GuillaumeGomez GuillaumeGomez Dec 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

No, it returns None, not "no values at all".

Also, I'm currently finishing to add missing doc examples, then I'll go through all missing urls. None and every other struct/enum/etc... should be linked.

Also, even it seems minor for you, it isn't for me since all these minor things people don't add, I have to after. And it's getting quite huge.

I don't ask it just to bother you or for extreme and dark reasons, it's really because there is a need out of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless I'm misunderstanding it the iter will either have length 1 and contain the value inside the Ok result, or it will have length zero. It never yields None, unless the result is Ok(None). That's what I was trying to say by it yields 'no values at all'.

https://play.rust-lang.org/?gist=3d6a9500b782e878fae356a08888838b&version=stable&backtrace=0

Copy link
Member

@GuillaumeGomez GuillaumeGomez Dec 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I see your point. Confusion on my side. Sorry about that... Therefore, no change is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, I could have been confused myself so I appreciate you checking.

I can send you a little cleanup pr later to linkify other instances. It sure would be nice if we can make it automatic later.

Copy link
Member

Choose a reason for hiding this comment

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

It's been discussed for a while. You can take a look at this issue if you want to take a look at the current status.

///
/// # Examples
///
/// Basic usage:
Expand All @@ -512,6 +514,8 @@ impl<T, E> Result<T, E> {
/// let x: Result<u32, &str> = Err("nothing!");
/// assert_eq!(x.iter().next(), None);
/// ```
///
/// [`Ok`]: enum.Result.html#variant.Ok
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub fn iter(&self) -> Iter<T> {
Expand All @@ -520,6 +524,8 @@ impl<T, E> Result<T, E> {

/// Returns a mutable iterator over the possibly contained value.
///
/// The iterator yields one value if the result is [`Ok`], otherwise none.
Copy link
Member

Choose a reason for hiding this comment

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

Otherwise None (with url).

///
/// # Examples
///
/// Basic usage:
Expand All @@ -535,6 +541,8 @@ impl<T, E> Result<T, E> {
/// let mut x: Result<u32, &str> = Err("nothing!");
/// assert_eq!(x.iter_mut().next(), None);
/// ```
///
/// [`Ok`]: enum.Result.html#variant.Ok
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub fn iter_mut(&mut self) -> IterMut<T> {
Expand Down Expand Up @@ -848,6 +856,8 @@ impl<T, E> IntoIterator for Result<T, E> {

/// Returns a consuming iterator over the possibly contained value.
///
/// The iterator yields one value if the result is [`Ok`], otherwise none.
Copy link
Member

Choose a reason for hiding this comment

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

Otherwise None (with url).

///
/// # Examples
///
/// Basic usage:
Expand All @@ -861,6 +871,8 @@ impl<T, E> IntoIterator for Result<T, E> {
/// let v: Vec<u32> = x.into_iter().collect();
/// assert_eq!(v, []);
/// ```
///
/// [`Ok`]: enum.Result.html#variant.Ok
#[inline]
fn into_iter(self) -> IntoIter<T> {
IntoIter { inner: self.ok() }
Expand Down Expand Up @@ -893,8 +905,13 @@ impl<'a, T, E> IntoIterator for &'a mut Result<T, E> {

/// An iterator over a reference to the [`Ok`] variant of a [`Result`].
///
/// The iterator yields one value if the result is [`Ok`], otherwise none.
Copy link
Member

Choose a reason for hiding this comment

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

Otherwise None (with url).

///
/// Created by [`Result::iter`].
///
/// [`Ok`]: enum.Result.html#variant.Ok
/// [`Result`]: enum.Result.html
/// [`Result::iter`]: enum.Result.html#method.iter
#[derive(Debug)]
#[stable(feature = "rust1", since = "1.0.0")]
pub struct Iter<'a, T: 'a> { inner: Option<&'a T> }
Expand Down Expand Up @@ -934,8 +951,11 @@ impl<'a, T> Clone for Iter<'a, T> {

/// An iterator over a mutable reference to the [`Ok`] variant of a [`Result`].
///
/// Created by [`Result::iter_mut`].
///
/// [`Ok`]: enum.Result.html#variant.Ok
/// [`Result`]: enum.Result.html
/// [`Result::iter_mut`]: enum.Result.html#method.iter_mut
#[derive(Debug)]
#[stable(feature = "rust1", since = "1.0.0")]
pub struct IterMut<'a, T: 'a> { inner: Option<&'a mut T> }
Expand Down Expand Up @@ -968,9 +988,12 @@ impl<'a, T> FusedIterator for IterMut<'a, T> {}
#[unstable(feature = "trusted_len", issue = "37572")]
unsafe impl<'a, A> TrustedLen for IterMut<'a, A> {}

/// An iterator over the value in a [`Ok`] variant of a [`Result`]. This struct is
/// created by the [`into_iter`] method on [`Result`][`Result`] (provided by
/// the [`IntoIterator`] trait).
/// An iterator over the value in a [`Ok`] variant of a [`Result`].
///
/// The iterator yields one value if the result is [`Ok`], otherwise none.
Copy link
Member

Choose a reason for hiding this comment

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

Otherwise None (with url).

///
/// This struct is created by the [`into_iter`] method on
/// [`Result`][`Result`] (provided by the [`IntoIterator`] trait).
///
/// [`Ok`]: enum.Result.html#variant.Ok
/// [`Result`]: enum.Result.html
Expand Down