Stabilize Seek::stream_position (feature seek_convenience)
#70904
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
FYI: a new discussion potentially arguing against stabilization just emerged in the tracking issue. |
|
Highfive is configured in https://github.com/rust-lang/highfive/blob/master/highfive/configs/rust-lang/rust.json#L6, and this list (in particular my absence from it ;)) is deliberate. r? @kennytm In general though the right time for a stabilization is after FCP has happened, because of cases like this:
|
Oh, oops. I simply checked https://www.rust-lang.org/governance/teams/library to see if kennytm is in the libs team, didn't see them listed there and assumed this was a highfive bug. Sorry!
I assumed that FCPs mostly happen on the PR and not the tracking issue. At least I think that that's mostly what I saw so far. I even considered asking for FCP on the tracking issues instead of creating a PR but decided against it because I thought FCP on PR is the "proper" way to do it. If you want, I can close this PR until FCP is complete. |
|
r? @dtolnay |
|
@rust-lang/libs:
@rfcbot fcp merge |
|
Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns: Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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. |
|
@rfcbot concern non-atomic-tell stream_position seems totally reasonable to me (maybe modulo a rename), but I'm not really a huge fan of stream_position being implemented by 3 seeks. It's unfortunately non-atomic and (almost?) every seekable construct of finite length should have a more "proper" method to return its length. |
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
|
|
55689e2
to
8a18fb0
Compare
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. The RFC will be merged soon. |
|
@bors r+ rollup |
|
📌 Commit 8a18fb0 has been approved by |
Rollup of 13 pull requests Successful merges: - rust-lang#70904 (Stabilize `Seek::stream_position` (feature `seek_convenience`)) - rust-lang#79951 (Refractor a few more types to `rustc_type_ir` ) - rust-lang#80868 (Print failure message on all tests that should panic, but don't) - rust-lang#81062 (Improve diagnostics for Precise Capture) - rust-lang#81277 (Make more traits of the From/Into family diagnostic items) - rust-lang#81284 (Make `-Z time-passes` less noisy) - rust-lang#81379 (Improve URLs handling) - rust-lang#81416 (Tweak suggestion for missing field in patterns) - rust-lang#81426 (const_evaluatable: expand abstract consts in try_unify) - rust-lang#81428 (compiletest: Add two more unit tests) - rust-lang#81430 (add const_evaluatable_checked test) - rust-lang#81433 (const_evaluatable: stop looking into type aliases) - rust-lang#81445 (Update cargo) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Most of the `seek_convenience` feature was stablized, but `stream_len` was not [0]. To minimize version incompatibilities here, just remove our `stream_len` functions for now and rely on the defaults. [0]: rust-lang/rust#70904
Most of the `seek_convenience` feature was stablized, but `stream_len` was not [0]. To minimize version incompatibilities here, just remove our `stream_len` functions for now and rely on the defaults. [0]: rust-lang/rust#70904
Tracking issue: #59359
Unresolved questions from tracking issue:
stream_lenforFile?" → we can do that in the future, this does not block stabilization.lenandposition?" → as noted in the tracking issue, both of these shorter names have problems (lenis usually a cheap getter,positionclashes withCursor). I do think the current names are perfectly fine.stream_positiontotell?" → as mentioned in the comment bringing this up,stream_positionis more descriptive. I don't thinktellwould be a good name.What remains to decide, is whether or not adding these methods is worth it.