docs: improve std::fs::read doc #114448

SteveLauC · 2023-08-04T05:05:30Z

What does this PR do

Rephrase a confusing sentence in the document of std::fs::read()

rustbot · 2023-08-04T05:05:37Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

gurry · 2023-08-04T05:21:35Z

Perhaps we should do more here and also explain what is so special about io::ErrorKind::Interrupted. What will happen if io::ErrorKind::Interrupted is encountered as opposed to errors "other than io::ErrorKind::Interrupted"? Why do we even bring up this error kind here?

As a reader the intent of this whole sentence is not clear to me.

SteveLauC · 2023-08-04T06:49:56Z

Perhaps we should do more here and also explain:

What is so special about io::ErrorKind::Interrupted? Why do we even bring up this error kind here?

Agree, IMO, any error that could be constructed from Error::from_raw_os_error(an_errno_that_could_be_returned_from_the_read_syscall) can potentially happen, Interrupted(EINTR) is just one of them

gurry · 2023-08-04T06:54:59Z

I see. If that is the case, why do we say it can throw errors other than Interrupted? Why not just say it can throw any error that could be constructed from Error::from_raw_os_error(an_errno_that_could_be_returned_from_the_read_syscall) (of course word that in a more user-friendly way).

Mark-Simulacrum · 2023-08-13T17:49:50Z

library/std/src/fs.rs

+/// Also, while reading, an error of a kind other than [`io::ErrorKind::Interrupted`]
+/// can be returned if it is encountered.


Suggested change

/// Also, while reading, an error of a kind other than [`io::ErrorKind::Interrupted`]

/// can be returned if it is encountered.

/// Reading from the file handles [`io::ErrorKind::Interrupted`] with automatic retries. See [`io::Read`] documentation for details.

https://doc.rust-lang.org/nightly/std/io/trait.Read.html#errors-1 has some further discussion of this -- just a suggestion on possible text here, I think this reads better than both master and the PR proposal. But happy to merge some other form.

Let's also update the similar text on line ~274.

Dexus0 · 2023-08-15T15:42:20Z

library/std/src/fs.rs

@@ -233,8 +233,8 @@ pub struct DirBuilder {
 /// This function will return an error if `path` does not already exist.
 /// Other errors may also be returned according to [`OpenOptions::open`].
 ///
-/// It will also return an error if it encounters while reading an error
-/// of a kind other than [`io::ErrorKind::Interrupted`].
+/// Reading from the file handles [`io::ErrorKind::Interrupted`] with automatic


Suggested change

/// Reading from the file handles [`io::ErrorKind::Interrupted`] with automatic

/// While reading from the file, handles [`io::ErrorKind::Interrupted`] with automatic

This makes more sense to me.

My suggestion seems a wee bit confusing without the next line for context...

What about adding a subject to the sentence:

While reading from the file, this function handles [io::ErrorKind::Interrupted] with automatic retries. See [io::Read] documentation for details.

Seems good to me.

SteveLauC · 2023-08-17T00:27:29Z

I just pushed the new code comment, ping me if you need me to squash the commits

Mark-Simulacrum · 2023-08-26T22:56:13Z

r=me with commits squashed

Dexus0 · 2023-08-31T12:06:44Z

r=me with commits squashed

@SteveLauC

SteveLauC · 2023-09-01T02:43:35Z

r=me with commits squashed

@SteveLauC

Does this instruction mean that I need to squash my commits? I thought it can be done automatically

saethlin · 2023-09-01T02:52:09Z

You need to squash your commits. GitHub can do it automatically if you merge via GitHub but the Rust organization does not use GitHub to merge commits. Rust uses an implementation of the same concept behind GitHub merge queues- you only merge if the merged state would pass the tests. Which means you must test merges one-at-a-time. Here's the queue for this repo: https://bors.rust-lang.org/queue/rust

People say "r=me with..." to indicate that anyone else with review permissions can send this off to the merge queue when the thing is done.

SteveLauC · 2023-09-01T02:58:28Z

You need to squash your commits. GitHub can do it automatically if you merge via GitHub but the Rust organization does not use GitHub to merge commits. Rust uses an implementation of the same concept behind GitHub merge queues- you only merge if the merged state would pass the tests. Which means you must test merges one-at-a-time. Here's the queue for this repo: https://bors.rust-lang.org/queue/rust

People say "r=me with..." to indicate that anyone else with review permissions can send this off to the merge queue when the thing is done.

Commits squashed, thanks for you detaild explanation:)

saethlin · 2023-09-01T03:52:39Z

@bors r=Mark-Simulacrum rollup=always

bors · 2023-09-01T03:52:41Z

📌 Commit 0e270b1 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

bors · 2023-09-01T05:25:48Z

⌛ Testing commit 0e270b1 with merge dc348db...

bors · 2023-09-01T07:10:15Z

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing dc348db to master...

rust-timer · 2023-09-01T23:24:47Z

Finished benchmarking commit (dc348db): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.3%	[1.3%, 1.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.5%	[-2.4%, -0.4%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.8%	[-2.4%, 1.3%]	4

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 631.304s -> 632.086s (0.12%)
Artifact size: 316.42 MiB -> 316.25 MiB (-0.06%)

rustbot assigned Mark-Simulacrum Aug 4, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 4, 2023

Mark-Simulacrum reviewed Aug 13, 2023

View reviewed changes

SteveLauC requested a review from Mark-Simulacrum August 14, 2023 01:21

This comment was marked as duplicate.

Sign in to view

Dexus0 suggested changes Aug 15, 2023

View reviewed changes

Dexus0 approved these changes Aug 18, 2023

View reviewed changes

docs: improve std::fs::read doc

0e270b1

SteveLauC force-pushed the std_fs_read_doc branch from ddfa40e to 0e270b1 Compare September 1, 2023 02:54

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 1, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 1, 2023

bors merged commit dc348db into rust-lang:master Sep 1, 2023
11 checks passed

rustbot added this to the 1.74.0 milestone Sep 1, 2023

SteveLauC deleted the std_fs_read_doc branch September 1, 2023 07:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: improve std::fs::read doc #114448

docs: improve std::fs::read doc #114448

SteveLauC commented Aug 4, 2023

rustbot commented Aug 4, 2023

gurry commented Aug 4, 2023 •

edited

Loading

SteveLauC commented Aug 4, 2023 •

edited

Loading

gurry commented Aug 4, 2023

Mark-Simulacrum Aug 13, 2023

SteveLauC Aug 14, 2023

This comment was marked as duplicate.

Dexus0 Aug 15, 2023

Dexus0 Aug 15, 2023

Dexus0 Aug 15, 2023

SteveLauC Aug 16, 2023

Dexus0 Aug 16, 2023

SteveLauC commented Aug 17, 2023

Mark-Simulacrum commented Aug 26, 2023

Dexus0 commented Aug 31, 2023

SteveLauC commented Sep 1, 2023

saethlin commented Sep 1, 2023

SteveLauC commented Sep 1, 2023

saethlin commented Sep 1, 2023

bors commented Sep 1, 2023

bors commented Sep 1, 2023

bors commented Sep 1, 2023

rust-timer commented Sep 1, 2023

		/// Also, while reading, an error of a kind other than [`io::ErrorKind::Interrupted`]
		/// can be returned if it is encountered.

	/// Also, while reading, an error of a kind other than [`io::ErrorKind::Interrupted`]
	/// can be returned if it is encountered.
	/// Reading from the file handles [`io::ErrorKind::Interrupted`] with automatic retries. See [`io::Read`] documentation for details.

	/// Reading from the file handles [`io::ErrorKind::Interrupted`] with automatic
	/// While reading from the file, handles [`io::ErrorKind::Interrupted`] with automatic

docs: improve std::fs::read doc #114448

docs: improve std::fs::read doc #114448

Conversation

SteveLauC commented Aug 4, 2023

What does this PR do

rustbot commented Aug 4, 2023

gurry commented Aug 4, 2023 • edited Loading

SteveLauC commented Aug 4, 2023 • edited Loading

gurry commented Aug 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

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

SteveLauC commented Aug 17, 2023

Mark-Simulacrum commented Aug 26, 2023

Dexus0 commented Aug 31, 2023

SteveLauC commented Sep 1, 2023

saethlin commented Sep 1, 2023

SteveLauC commented Sep 1, 2023

saethlin commented Sep 1, 2023

bors commented Sep 1, 2023

bors commented Sep 1, 2023

bors commented Sep 1, 2023

rust-timer commented Sep 1, 2023

Overall result: no relevant changes - no action needed

gurry commented Aug 4, 2023 •

edited

Loading

SteveLauC commented Aug 4, 2023 •

edited

Loading