Try to improve the documentation of `filter()` and `filter_map()`. #75949

vext01 · 2020-08-26T16:48:16Z

I believe the documentation is currently a little misleading.

For example, in the docs for filter():

If the closure returns false, it will try again, and call the closure on
the next element, seeing if it passes the test.

This kind of implies that if the closure returns true then we don't "try
again" and no further elements are considered. In actuality that's not the
case, every element is tried regardless of what happened with the previous
element.

This change tries to clarify that by removing the uses of "try again"
altogether.

rust-highfive · 2020-08-26T16:48:19Z

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

vext01 · 2020-08-26T16:48:59Z

I've just noticed that I wrapped my lines longer than before. Let me know if that's an issue and I'll fix it.

Dylan-DPC-zz · 2020-08-26T23:20:43Z

r? @steveklabnik

steveklabnik · 2020-08-28T14:21:42Z

I've just noticed that I wrapped my lines longer than before. Let me know if that's an issue and I'll fix it.

What column are they wrapped to? Standard library, IIRC, uses 80.

vext01 · 2020-08-28T23:03:12Z

Re-wrapped to 80 and made the docs for filter_map() yet more concise.

What do you think?

vext01 · 2020-08-28T23:05:25Z

It occurs to me that it might be better to say returned iterator instead of filtered result? Thoughts?

steveklabnik

I think this looks good, but yeah, I also hear you about the "returned iterator". Hmmm...

vext01 · 2020-09-01T09:32:04Z

Having thought a little more, I prefer "returned iterator". Fix pushed.

OK to squash?

library/core/src/iter/traits/iterator.rs

bors · 2020-09-03T02:37:02Z

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

vext01 · 2020-09-03T08:54:19Z

Rebased to resolve merge conflict. Another change updated the links which I had removed anyway.

hbina · 2020-09-03T14:37:50Z

Why not something like "The returned iterator will only contain elements that returns true when applied to the predicate". Short and no double negatives with "false" and "not"

vext01 · 2020-09-03T14:40:24Z

I'd go for a slight tweak on that:

The returned iterator will only contain the elements for which the closure returns true.

(And we've already said that each element will be passed to the closure)

jyn514 · 2020-09-03T15:31:39Z

You removed some links also that need to be added back:

error: unresolved link to `filter`
   --> library/core/src/iter/traits/iterator.rs:743:44
    |
743 |     /// Here's the same example, but with [`filter`] and [`map`]:
    |                                            ^^^^^^^^ unresolved link
    |
    = note: `-D broken-intra-doc-links` implied by `-D warnings`
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`

error: unresolved link to `map`
   --> library/core/src/iter/traits/iterator.rs:743:59
    |
743 |     /// Here's the same example, but with [`filter`] and [`map`]:
    |                                                           ^^^^^ unresolved link
    |
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`

I still consider #75949 (comment) a blocking concern though.

vext01 · 2020-09-04T11:36:48Z

I've had a shot at addressing comments.

There's a curious merge conflict (for a path I've not touched), but we can look at that later.

Are we getting any closer?

jyn514 · 2020-09-04T12:06:17Z

There's a curious merge conflict (for a path I've not touched), but we can look at that later.

You updated submodules, I'm not quite sure how to fix it. Maybe git reset master src/{doc,edition-guide,reference,rust-by-example} src/tools/{cargo,miri,rust-analyzer}?

jyn514 · 2020-09-04T12:07:44Z

library/core/src/iter/traits/iterator.rs

+    /// Given an element the closure must return `Some(value)` or `None`. The
+    /// returned iterator will only yield a value when the closure returns
+    /// `Some`. The values inside the `Some`s are yielded.


This seems a little verbose, but I'm not sure how better to word it. It seems like it's saying the same thing 3 times sort of.

I spent quite some time trying to get these three sentences down to 2 (or even better one), but failed.

It's hard to explain concisely that it is not the element that is returned, but the innards of the Some that is returned.

I'm open to suggestions, but also I'd like to move forward. You can always tweak this in a later PR.

Would something like this be any better?

The returned iterator yields only the values for which the supplied closure returns Some(value).

Ooh, I like that!

library/core/src/iter/traits/iterator.rs

jyn514 · 2020-09-05T16:14:50Z

r=me without the submodule updates. Not sure how exactly to fix those ... maybe something like this?

git checkout filter-docs
git reset fd001558c8f077da2e9e645c6667881b48524e72
git add library/core/src/iter/traits/iterator.rs
git commit -m "Address comments"
git push -f

vext01 · 2020-09-05T16:22:53Z

@jyn514 I think we raced. I made a suggestion in the above thread at the same time as it was marked resolved.

vext01 · 2020-09-05T17:18:04Z

I think this should be good.

Assuming all is well, can I squash?

rust-log-analyzer · 2020-09-05T17:23:51Z

The job mingw-check of your PR failed (pretty log, 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.

Initialized empty Git repository in /home/runner/work/rust/rust/.git/
##[command]git remote add origin https://github.com/rust-lang/rust
##[command]git config gc.auto 0
##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/75949/merge:refs/remotes/pull/75949/merge

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 @rust-lang/infra. (Feature Requests)

jyn514 · 2020-09-05T17:31:15Z

Looks good to me but you still need to undo the submodule changes: #75949 (comment)

I believe the documentation is currently a little misleading. For example, in the docs for `filter()`: > If the closure returns `false`, it will try again, and call the closure on > the next element, seeing if it passes the test. This kind of implies that if the closure returns true then we *don't* "try again" and no further elements are considered. In actuality that's not the case, every element is tried regardless of what happened with the previous element. This change tries to clarify that by removing the uses of "try again" altogether.

vext01 · 2020-09-05T19:02:25Z

Ugh. I thought I had removed those.

I think this is the desired result (with a squash also).

jyn514 · 2020-09-05T19:25:27Z

@bors r+ rollup

Thanks for the PR! (and for persisting through the trouble with submodules)

bors · 2020-09-05T19:25:28Z

📌 Commit 8af85fa has been approved by jyn514

Try to improve the documentation of `filter()` and `filter_map()`. I believe the documentation is currently a little misleading. For example, in the docs for `filter()`: > If the closure returns `false`, it will try again, and call the closure on > the next element, seeing if it passes the test. This kind of implies that if the closure returns true then we *don't* "try again" and no further elements are considered. In actuality that's not the case, every element is tried regardless of what happened with the previous element. This change tries to clarify that by removing the uses of "try again" altogether.

bors · 2020-09-06T12:30:26Z

⌛ Testing commit 8af85fa with merge 642ebd19e1bccb1179d6acee8af7e56f8920b242...

bors · 2020-09-06T12:34:26Z

💔 Test failed - checks-azure

rust-log-analyzer · 2020-09-06T12:38:50Z

Your PR failed (pretty log, 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.

##[group]Run exit 1
exit 1
shell: /bin/bash --noprofile --norc -e -o pipefail {0}
##[endgroup]
##[error]Process completed with exit code 1.

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 @rust-lang/infra. (Feature Requests)

jyn514 · 2020-09-06T12:42:35Z

@bors retry

network error:

curl: (56) SSLRead() return error -9806
clang+llvm-10.0.0-x86_64-apple-darwin/include/clang/AST/ExprObjC.h: Lzma library error:  No progress is possible
tar: Error exit delayed from previous errors.

Try to improve the documentation of `filter()` and `filter_map()`. I believe the documentation is currently a little misleading. For example, in the docs for `filter()`: > If the closure returns `false`, it will try again, and call the closure on > the next element, seeing if it passes the test. This kind of implies that if the closure returns true then we *don't* "try again" and no further elements are considered. In actuality that's not the case, every element is tried regardless of what happened with the previous element. This change tries to clarify that by removing the uses of "try again" altogether.

bors · 2020-09-06T18:26:39Z

⌛ Testing commit 8af85fa with merge 5d74e88...

bors · 2020-09-06T20:27:44Z

☀️ Test successful - checks-actions, checks-azure
Approved by: jyn514
Pushing 5d74e88 to master...

rust-highfive assigned cramertj Aug 26, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 26, 2020

rust-highfive assigned steveklabnik and unassigned cramertj Aug 26, 2020

steveklabnik reviewed Aug 31, 2020

View reviewed changes

jyn514 added A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools A-iterators Area: Iterators labels Sep 3, 2020

jyn514 reviewed Sep 3, 2020

View reviewed changes

library/core/src/iter/traits/iterator.rs Outdated Show resolved Hide resolved

vext01 force-pushed the filter-docs branch from d227e55 to fd00155 Compare September 3, 2020 08:53

jyn514 reviewed Sep 4, 2020

View reviewed changes

vext01 force-pushed the filter-docs branch from bc8902e to a40d9c7 Compare September 5, 2020 17:16

vext01 force-pushed the filter-docs branch from a40d9c7 to 8af85fa Compare September 5, 2020 19:01

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 Sep 5, 2020

Dylan-DPC-zz mentioned this pull request Sep 5, 2020

Rollup of 11 pull requests #76394

Closed

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 6, 2020

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 Sep 6, 2020

Dylan-DPC-zz mentioned this pull request Sep 6, 2020

Rollup of 18 pull requests #76414

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 6, 2020

bors merged commit 5d74e88 into rust-lang:master Sep 6, 2020

rustbot added this to the 1.48.0 milestone Sep 6, 2020

jyn514 mentioned this pull request Sep 6, 2020

Add a section on how to use git rust-lang/rustc-dev-guide#850

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try to improve the documentation of `filter()` and `filter_map()`. #75949

Try to improve the documentation of `filter()` and `filter_map()`. #75949

vext01 commented Aug 26, 2020

rust-highfive commented Aug 26, 2020

vext01 commented Aug 26, 2020

Dylan-DPC-zz commented Aug 26, 2020

steveklabnik commented Aug 28, 2020

vext01 commented Aug 28, 2020

vext01 commented Aug 28, 2020

steveklabnik left a comment

vext01 commented Sep 1, 2020

bors commented Sep 3, 2020

vext01 commented Sep 3, 2020 •

edited

Loading

hbina commented Sep 3, 2020

vext01 commented Sep 3, 2020

jyn514 commented Sep 3, 2020

vext01 commented Sep 4, 2020

jyn514 commented Sep 4, 2020

jyn514 Sep 4, 2020

vext01 Sep 5, 2020

vext01 Sep 5, 2020

jyn514 Sep 5, 2020

jyn514 commented Sep 5, 2020

vext01 commented Sep 5, 2020

vext01 commented Sep 5, 2020

rust-log-analyzer commented Sep 5, 2020

jyn514 commented Sep 5, 2020

vext01 commented Sep 5, 2020

jyn514 commented Sep 5, 2020

bors commented Sep 5, 2020

bors commented Sep 6, 2020

bors commented Sep 6, 2020

rust-log-analyzer commented Sep 6, 2020

jyn514 commented Sep 6, 2020

bors commented Sep 6, 2020

bors commented Sep 6, 2020

Try to improve the documentation of filter() and filter_map(). #75949

Try to improve the documentation of filter() and filter_map(). #75949

Conversation

vext01 commented Aug 26, 2020

rust-highfive commented Aug 26, 2020

vext01 commented Aug 26, 2020

Dylan-DPC-zz commented Aug 26, 2020

steveklabnik commented Aug 28, 2020

vext01 commented Aug 28, 2020

vext01 commented Aug 28, 2020

steveklabnik left a comment

Choose a reason for hiding this comment

vext01 commented Sep 1, 2020

bors commented Sep 3, 2020

vext01 commented Sep 3, 2020 • edited Loading

hbina commented Sep 3, 2020

vext01 commented Sep 3, 2020

jyn514 commented Sep 3, 2020

vext01 commented Sep 4, 2020

jyn514 commented Sep 4, 2020

jyn514 Sep 4, 2020

Choose a reason for hiding this comment

vext01 Sep 5, 2020

Choose a reason for hiding this comment

vext01 Sep 5, 2020

Choose a reason for hiding this comment

jyn514 Sep 5, 2020

Choose a reason for hiding this comment

jyn514 commented Sep 5, 2020

vext01 commented Sep 5, 2020

vext01 commented Sep 5, 2020

rust-log-analyzer commented Sep 5, 2020

jyn514 commented Sep 5, 2020

vext01 commented Sep 5, 2020

jyn514 commented Sep 5, 2020

bors commented Sep 5, 2020

bors commented Sep 6, 2020

bors commented Sep 6, 2020

rust-log-analyzer commented Sep 6, 2020

jyn514 commented Sep 6, 2020

bors commented Sep 6, 2020

bors commented Sep 6, 2020

Try to improve the documentation of `filter()` and `filter_map()`. #75949

Try to improve the documentation of `filter()` and `filter_map()`. #75949

vext01 commented Sep 3, 2020 •

edited

Loading