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

Allow setting the limit on std::io::Take. #42697

Merged
merged 1 commit into from
Jul 12, 2017

Conversation

Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Jun 16, 2017

Fixes #27269.

@Mark-Simulacrum Mark-Simulacrum added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 16, 2017
@rust-highfive
Copy link
Collaborator

r? @sfackler

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

@Mark-Simulacrum
Copy link
Member Author

Arguably this can also be not implemented since out-of-tree implementation should involve no more than take.into_inner().take(new_limit), though this is more ergonomic (&mut self vs self).

@sfackler
Copy link
Member

This seems reasonable to me - might be worth explicitly mentioning that it doesn't "reset" the count of what's been read?

@rfcbot fcp merge

@Mark-Simulacrum
Copy link
Member Author

I'll fix the code if we decide to move forward with this, hopefully landing after #42612.

@rfcbot
Copy link

rfcbot commented Jun 16, 2017

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jun 17, 2017
@alexcrichton
Copy link
Member

ping @aturon / @brson on checkboxes

@Mark-Simulacrum
Copy link
Member Author

@brson I believe this is only waiting on approval from you, will push a fix for the nit @sfackler noted with regards to docs.

@carols10cents
Copy link
Member

ping @brson, still waiting for your checkbox here!

@@ -1761,6 +1761,35 @@ impl<T> Take<T> {
#[stable(feature = "rust1", since = "1.0.0")]
pub fn limit(&self) -> u64 { self.limit }

/// Sets the number of bytes that can be read before this instance will
/// return EOF. This is the same as constructing a new `Take` instance, so
/// the amount of bytes read or the previous limit value do not affect the
Copy link
Member

Choose a reason for hiding this comment

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

"do not affect the effect" is a somewhat confusing phrasing. Also missing period.

@aturon
Copy link
Member

aturon commented Jul 10, 2017

@Mark-Simulacrum

Left a nit, otherwise r=me

@Mark-Simulacrum
Copy link
Member Author

Fixed the nit (I think). @brson hasn't checked off yet, so I'm hesitant to r+ without that.

@aturon
Copy link
Member

aturon commented Jul 12, 2017

@bors: r+

@Mark-Simulacrum for PRs (rather than RFCs), after a week if most folks have signed off we go forward.

@bors
Copy link
Contributor

bors commented Jul 12, 2017

📌 Commit 7109d03 has been approved by aturon

@bors
Copy link
Contributor

bors commented Jul 12, 2017

⌛ Testing commit 7109d03 with merge 8ac29bd...

bors added a commit that referenced this pull request Jul 12, 2017
Allow setting the limit on std::io::Take.

Fixes #27269.
@bors
Copy link
Contributor

bors commented Jul 12, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: aturon
Pushing 8ac29bd to master...

@bors bors merged commit 7109d03 into rust-lang:master Jul 12, 2017
@Mark-Simulacrum Mark-Simulacrum deleted the take-limit branch January 17, 2018 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants